)]}'
{"neutron/conf/plugins/ml2/config.py":[{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"f53590b12891a91e839d0548c3136351777cbd19","unresolved":true,"context_lines":[{"line_number":66,"context_line":"               default\u003d4,"},{"line_number":67,"context_line":"               help\u003d_(\"IP version of all overlay (tunnel) network endpoints. \""},{"line_number":68,"context_line":"                      \"Use a value of 4 for IPv4 or 6 for IPv6.\")),"},{"line_number":69,"context_line":"    cfg.IntOpt(\u0027obj_change_worker_timeout\u0027,"},{"line_number":70,"context_line":"               default\u003d1,"},{"line_number":71,"context_line":"               help\u003d_(\"Timeout on queue of ovo_rpc._ObjectChangeHandler, \""},{"line_number":72,"context_line":"                      \"after which worker thread will finish. \""}],"source_content_type":"text/x-python","patch_set":4,"id":"7165f767_46ff76fc","line":69,"range":{"start_line":69,"start_character":15,"end_line":69,"end_character":42},"updated":"2021-05-17 08:00:43.000000000","message":"why need a config for this? Why would operator want to change this? IMO it\u0027s quite a low level implementation detail","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"b43de9359686e9b512bd16ce54aca7fa460d1bf5","unresolved":false,"context_lines":[{"line_number":66,"context_line":"               default\u003d4,"},{"line_number":67,"context_line":"               help\u003d_(\"IP version of all overlay (tunnel) network endpoints. \""},{"line_number":68,"context_line":"                      \"Use a value of 4 for IPv4 or 6 for IPv6.\")),"},{"line_number":69,"context_line":"    cfg.IntOpt(\u0027obj_change_worker_timeout\u0027,"},{"line_number":70,"context_line":"               default\u003d1,"},{"line_number":71,"context_line":"               help\u003d_(\"Timeout on queue of ovo_rpc._ObjectChangeHandler, \""},{"line_number":72,"context_line":"                      \"after which worker thread will finish. \""}],"source_content_type":"text/x-python","patch_set":4,"id":"b275ffc5_49adbbdb","line":69,"range":{"start_line":69,"start_character":15,"end_line":69,"end_character":42},"in_reply_to":"017c9381_8c0ca07d","updated":"2021-05-24 12:24:09.000000000","message":"I didn\u0027t want to hardcode it, but yeah, I agree. Fixed","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"93ce542cdcad3054d6d319109da6d19d697790ef","unresolved":true,"context_lines":[{"line_number":66,"context_line":"               default\u003d4,"},{"line_number":67,"context_line":"               help\u003d_(\"IP version of all overlay (tunnel) network endpoints. \""},{"line_number":68,"context_line":"                      \"Use a value of 4 for IPv4 or 6 for IPv6.\")),"},{"line_number":69,"context_line":"    cfg.IntOpt(\u0027obj_change_worker_timeout\u0027,"},{"line_number":70,"context_line":"               default\u003d1,"},{"line_number":71,"context_line":"               help\u003d_(\"Timeout on queue of ovo_rpc._ObjectChangeHandler, \""},{"line_number":72,"context_line":"                      \"after which worker thread will finish. \""}],"source_content_type":"text/x-python","patch_set":4,"id":"017c9381_8c0ca07d","line":69,"range":{"start_line":69,"start_character":15,"end_line":69,"end_character":42},"in_reply_to":"7165f767_46ff76fc","updated":"2021-05-19 17:09:29.000000000","message":"Agree with this. Let\u0027s keep the config knob set as reduced as possible.","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"}],"neutron/plugins/ml2/ovo_rpc.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b023f170ebf6c2bb8c0459ef7f706752cb4d892c","unresolved":true,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"class _ObjectChangeHandler(object):"},{"line_number":38,"context_line":"    def __init__(self, resource, object_class, resource_push_api):"},{"line_number":39,"context_line":"        self._resource \u003d resource"}],"source_content_type":"text/x-python","patch_set":1,"id":"e02c3795_4beb762b","line":36,"updated":"2021-04-28 15:21:58.000000000","message":"Still waiting for the CI but that\u0027s an interesting approach and initially I\u0027m ok with it.\n\nOne thread processing all events should be more efficient than spawning one per OVO, due to the Python (lack of) multithreading.","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b023f170ebf6c2bb8c0459ef7f706752cb4d892c","unresolved":true,"context_lines":[{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    def wait(self):"},{"line_number":55,"context_line":"        \"\"\"Waits for all outstanding events to be dispatched.\"\"\""},{"line_number":56,"context_line":"        self._resources_to_push.join()"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def _is_session_semantic_violated(self, context, resource, event):"},{"line_number":59,"context_line":"        \"\"\"Return True and print an ugly error on transaction violation."}],"source_content_type":"text/x-python","patch_set":1,"id":"90f0dcc0_35ce8b4a","line":56,"range":{"start_line":56,"start_character":13,"end_line":56,"end_character":31},"updated":"2021-04-28 15:21:58.000000000","message":"Shouldn\u0027t be \"self._worker.join()\" ?","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"886757d3ac9398269f3dea82e9a490b560834660","unresolved":true,"context_lines":[{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    def wait(self):"},{"line_number":55,"context_line":"        \"\"\"Waits for all outstanding events to be dispatched.\"\"\""},{"line_number":56,"context_line":"        self._resources_to_push.join()"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    def _is_session_semantic_violated(self, context, resource, event):"},{"line_number":59,"context_line":"        \"\"\"Return True and print an ugly error on transaction violation."}],"source_content_type":"text/x-python","patch_set":1,"id":"a83cd055_ad17c3df","line":56,"range":{"start_line":56,"start_character":13,"end_line":56,"end_character":31},"in_reply_to":"90f0dcc0_35ce8b4a","updated":"2021-04-28 17:19:55.000000000","message":"To assure we processed all items in queue I decided to use queue.task_done() and queue.join().\nSince this is daemon thread we don\u0027t care about joining it as long as it finished all items in queue.\n\nBTW while this approach easily solves initial problem, it would make fixing TODO by kevinbenton about batch get_objects call quite a bit harder since I can\u0027t easily get all items from queue at once. So I guess I\u0027ll end up doing what you suggested in other comment and use a dict instead of queue as it was before.","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b023f170ebf6c2bb8c0459ef7f706752cb4d892c","unresolved":true,"context_lines":[{"line_number":93,"context_line":"        # TODO(kevinbenton): now that we are batching these, convert to a"},{"line_number":94,"context_line":"        # single get_objects call for all of them"},{"line_number":95,"context_line":"        while True:"},{"line_number":96,"context_line":"            resource_id, context_dict \u003d self._resources_to_push.get()"},{"line_number":97,"context_line":"            context \u003d n_ctx.Context.from_dict(context_dict)"},{"line_number":98,"context_line":"            # attempt to get regardless of event type so concurrent delete"},{"line_number":99,"context_line":"            # after create/update is the same code-path as a delete event"}],"source_content_type":"text/x-python","patch_set":1,"id":"671f0ea4_9ed3c157","line":96,"range":{"start_line":96,"start_character":40,"end_line":96,"end_character":69},"updated":"2021-04-28 15:21:58.000000000","message":"In a single thread architecture, I would make this conditional with a timeout, for example 1 sec.\n\nIf nothing is stored in \"_resources_to_push\", an exception will be thrown. This could be captured and continue the processing. That will prevent from hanging this thread when shutting down the server.\n\nThe loop condition \"while True\" should also depend on a variable that signals the process exit. You can catch the exit event with something like [1]. At this point you can unset this flag, the loop will end and L56 in this patch will finish.\n\n[1]https://github.com/openstack/neutron/blob/1ad9ca56b07ffdc9f7e0bc6a62af61961b9128eb/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py#L252","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"886757d3ac9398269f3dea82e9a490b560834660","unresolved":true,"context_lines":[{"line_number":93,"context_line":"        # TODO(kevinbenton): now that we are batching these, convert to a"},{"line_number":94,"context_line":"        # single get_objects call for all of them"},{"line_number":95,"context_line":"        while True:"},{"line_number":96,"context_line":"            resource_id, context_dict \u003d self._resources_to_push.get()"},{"line_number":97,"context_line":"            context \u003d n_ctx.Context.from_dict(context_dict)"},{"line_number":98,"context_line":"            # attempt to get regardless of event type so concurrent delete"},{"line_number":99,"context_line":"            # after create/update is the same code-path as a delete event"}],"source_content_type":"text/x-python","patch_set":1,"id":"97264f32_a02879ae","line":96,"range":{"start_line":96,"start_character":40,"end_line":96,"end_character":69},"in_reply_to":"671f0ea4_9ed3c157","updated":"2021-04-28 17:19:55.000000000","message":"My initial approach as seen in this patch assumes two things:\n* we are blocking on queue.get() to not waste cpu time if queue is empty\n* and we are using daemon thread which won\u0027t block program exit\n\nI agree that \"while True\" doesn\u0027t look great, but registering atexit \u0026 signal handlers to stop the loop seems like overkill to me.\nAlternatively what do you think about stopping thread on \"wait\" call? Since this is what we use to ensure clean stop of this class.","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"d7eb852bc5dcf14020b46c64f02488c949ba5b17","unresolved":true,"context_lines":[{"line_number":110,"context_line":"            else:"},{"line_number":111,"context_line":"                rpc_event \u003d rpc_events.UPDATED"},{"line_number":112,"context_line":"            self._resource_push_api.push(context, [obj], rpc_event)"},{"line_number":113,"context_line":"            self._resources_to_push.task_done()"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"    def _extract_resource_id(self, callback_kwargs):"},{"line_number":116,"context_line":"        id_kwarg \u003d \u0027%s_id\u0027 % self._resource"}],"source_content_type":"text/x-python","patch_set":1,"id":"db39a3bd_1e7e75d5","line":113,"range":{"start_line":113,"start_character":12,"end_line":113,"end_character":47},"updated":"2021-04-30 07:37:16.000000000","message":"This can throw an exception (ValuError) don\u0027t you need to catch that?","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"86f1322987684fa3f0fdb1f2e5362e31f2526b04","unresolved":true,"context_lines":[{"line_number":110,"context_line":"            else:"},{"line_number":111,"context_line":"                rpc_event \u003d rpc_events.UPDATED"},{"line_number":112,"context_line":"            self._resource_push_api.push(context, [obj], rpc_event)"},{"line_number":113,"context_line":"            self._resources_to_push.task_done()"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"    def _extract_resource_id(self, callback_kwargs):"},{"line_number":116,"context_line":"        id_kwarg \u003d \u0027%s_id\u0027 % self._resource"}],"source_content_type":"text/x-python","patch_set":1,"id":"37674d06_35b20d2b","line":113,"range":{"start_line":113,"start_character":12,"end_line":113,"end_character":47},"in_reply_to":"db39a3bd_1e7e75d5","updated":"2021-05-07 10:07:04.000000000","message":"ValueError can only be thrown here if we would call it more times than there were items in the queue. Since we have blocking call to get() earlier that would never happen.\nBut I agree there should be some big try-except so the thread won\u0027t die if some exception happens - I\u0027m working on it.","commit_id":"1b8f71a3dc79ed6c830d6f2181096caac296a8a9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"93ce542cdcad3054d6d319109da6d19d697790ef","unresolved":true,"context_lines":[{"line_number":48,"context_line":"            registry.subscribe(self.handle_event, resource, event)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def _start_worker(self):"},{"line_number":51,"context_line":"        if self._worker is not None and self._worker.is_alive():"},{"line_number":52,"context_line":"            return"},{"line_number":53,"context_line":"        self._worker \u003d threading.Thread("},{"line_number":54,"context_line":"            target\u003dself.dispatch_events,"}],"source_content_type":"text/x-python","patch_set":4,"id":"78419c53_61d7aa08","line":51,"range":{"start_line":51,"start_character":8,"end_line":51,"end_character":64},"updated":"2021-05-19 17:09:29.000000000","message":"Let me ask you: why not having a permanent running thread, attending to \"_resources_to_push\"? What is the benefit of this implementation (spawning a new thread when an old one finish)?","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"b43de9359686e9b512bd16ce54aca7fa460d1bf5","unresolved":false,"context_lines":[{"line_number":48,"context_line":"            registry.subscribe(self.handle_event, resource, event)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def _start_worker(self):"},{"line_number":51,"context_line":"        if self._worker is not None and self._worker.is_alive():"},{"line_number":52,"context_line":"            return"},{"line_number":53,"context_line":"        self._worker \u003d threading.Thread("},{"line_number":54,"context_line":"            target\u003dself.dispatch_events,"}],"source_content_type":"text/x-python","patch_set":4,"id":"11f9629e_01e59621","line":51,"range":{"start_line":51,"start_character":8,"end_line":51,"end_character":64},"in_reply_to":"78419c53_61d7aa08","updated":"2021-05-24 12:24:09.000000000","message":"I probably misunderstood why you wanted timeout on queue.get() in dispatch_events.\nI\u0027ll change it back.","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"93ce542cdcad3054d6d319109da6d19d697790ef","unresolved":true,"context_lines":[{"line_number":100,"context_line":"        # single get_objects call for all of them"},{"line_number":101,"context_line":"        try:"},{"line_number":102,"context_line":"            while True:"},{"line_number":103,"context_line":"                resource_id, context_dict \u003d self._resources_to_push.get("},{"line_number":104,"context_line":"                    timeout\u003dcfg.CONF.ml2.obj_change_worker_timeout)"},{"line_number":105,"context_line":"                context \u003d n_ctx.Context.from_dict(context_dict)"},{"line_number":106,"context_line":"                # attempt to get regardless of event type so concurrent delete"}],"source_content_type":"text/x-python","patch_set":4,"id":"6bb054c2_493b5f14","line":103,"updated":"2021-05-19 17:09:29.000000000","message":"Similar comment as in PS1: what if we have too many items in \"_resources_to_push\" and this thread never ends when the main process is finished? We should set a flag when atexit and SIGTERM [1]; the while loop should depend on this flag.\n\nhttps://github.com/openstack/neutron/blob/1ad9ca56b07ffdc9f7e0bc6a62af61961b9128eb/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py#L252-L253","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"b43de9359686e9b512bd16ce54aca7fa460d1bf5","unresolved":true,"context_lines":[{"line_number":100,"context_line":"        # single get_objects call for all of them"},{"line_number":101,"context_line":"        try:"},{"line_number":102,"context_line":"            while True:"},{"line_number":103,"context_line":"                resource_id, context_dict \u003d self._resources_to_push.get("},{"line_number":104,"context_line":"                    timeout\u003dcfg.CONF.ml2.obj_change_worker_timeout)"},{"line_number":105,"context_line":"                context \u003d n_ctx.Context.from_dict(context_dict)"},{"line_number":106,"context_line":"                # attempt to get regardless of event type so concurrent delete"}],"source_content_type":"text/x-python","patch_set":4,"id":"7f95b8ac_b3251fb7","line":103,"in_reply_to":"6bb054c2_493b5f14","updated":"2021-05-24 12:24:09.000000000","message":"Maybe I\u0027m missing something, so I would be glad if you could explain why do you think atexit \u0026 signal handler should be used here. In my understanding:\n\nPython will kill this thread automatically at process exit, as soon as main thread is finished and without waiting for anything, no matter what it\u0027s doing and how many items are in the queue, because it\u0027s marked as daemon.\n\nHowever, since main thread calls wait() method before exiting (in case of normal exit) it will actually process all items in queue before exiting - this is identical to behavior before this patchset. After that process will end.\n\nEven now, though, when I added cleanup mechanism based on atexit and signal (similar to how futurist is doing it), it doesn\u0027t change much.\n\nAtexit / signal handler sets stop event and then main thread ends. At that time loop is blocked by queue.get() call.\nSince main thread ended, all daemon threads are disposed of and process is finished.\nThere won\u0027t even be a chance to check what was the state of stop event because get() call didn\u0027t have a chance to timeout before it was killed.","commit_id":"1f2865e05cfc192b746e9a49a28e16ad91ad0474"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"5286d1c6ed2b31a13dee62673c491af2278e71e0","unresolved":true,"context_lines":[{"line_number":202,"context_line":"        worker.stop()"},{"line_number":203,"context_line":""},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"atexit.register(_clean_up)"},{"line_number":206,"context_line":"signal.signal(signal.SIGINT, _clean_up)"},{"line_number":207,"context_line":"signal.signal(signal.SIGTERM, _clean_up)"}],"source_content_type":"text/x-python","patch_set":10,"id":"e5d7df7c_0868c10d","line":205,"updated":"2021-06-25 07:25:40.000000000","message":"nit: these methods (atexit.register, signal.signal) can register a class method too, instead of creating this \"_to_be_cleaned\" set and implementing the \"_clean_up\" method outside the _ObjectChangeHandler class.\n\nIn any case, that works too.","commit_id":"d861807233e660391f100a5ff3db47c6f04ec300"},{"author":{"_account_id":12825,"name":"Szymon Wróblewski","email":"szymon.wroblewski@ovhcloud.com","username":"bluex"},"change_message_id":"af4ac2bda5b4b053c9a2e697b779f3248906249f","unresolved":false,"context_lines":[{"line_number":202,"context_line":"        worker.stop()"},{"line_number":203,"context_line":""},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"atexit.register(_clean_up)"},{"line_number":206,"context_line":"signal.signal(signal.SIGINT, _clean_up)"},{"line_number":207,"context_line":"signal.signal(signal.SIGTERM, _clean_up)"}],"source_content_type":"text/x-python","patch_set":10,"id":"12dc4203_efe22231","line":205,"in_reply_to":"e5d7df7c_0868c10d","updated":"2021-06-25 12:28:07.000000000","message":"Good idea, fixed","commit_id":"d861807233e660391f100a5ff3db47c6f04ec300"}]}