)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ec8adc6b81d08343330818b3c710492ef952843a","unresolved":false,"context_lines":[{"line_number":9,"context_line":"The L3 agent will now notify its drivers when a router is created,"},{"line_number":10,"context_line":"updated or deleted."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Driver registration is handled by calling driver.register(driver_manager)."},{"line_number":13,"context_line":"Registering the same type of driver twice is forbidden. Drivers are"},{"line_number":14,"context_line":"singletons."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a890539_1d2ebffa","line":12,"updated":"2014-12-01 19:59:17.000000000","message":"I like the decoupling from the separate driver notification class.\n\nShould this patch and 136549 be merged together (I have no problem with you adding this to the other commit, rather than having an added dependency).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":18,"context_line":"  fails, the other drivers don\u0027t get notified, and the entire operation fails."},{"line_number":19,"context_line":"  Should each driver notification be surrounded in a try/except block,"},{"line_number":20,"context_line":"  so that other drivers may be notified? Should we re-raise at the end of the"},{"line_number":21,"context_line":"  notification process?"},{"line_number":22,"context_line":"* Sending notifications is currently intertwined in the L3 agent code,"},{"line_number":23,"context_line":"  even adding logic such as: Is this router new, or is merely an update?"},{"line_number":24,"context_line":"  Is there a way to decouple this code from the agent, or otherwise make it"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a890539_410cd93b","line":21,"updated":"2014-12-02 00:43:14.000000000","message":"I think we should follow this on with another patch which would add this robustness.  However, I agree with what you\u0027ve done here.  Refactoring shouldn\u0027t change behavior in general.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":22,"context_line":"* Sending notifications is currently intertwined in the L3 agent code,"},{"line_number":23,"context_line":"  even adding logic such as: Is this router new, or is merely an update?"},{"line_number":24,"context_line":"  Is there a way to decouple this code from the agent, or otherwise make it"},{"line_number":25,"context_line":"  less invasive?"},{"line_number":26,"context_line":"* Should events be sent in dedicated Eventlet threads? I think that if needed,"},{"line_number":27,"context_line":"  this functionality can be added at a later date."},{"line_number":28,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a890539_4615cf47","line":25,"updated":"2014-12-02 00:43:14.000000000","message":"I think that is the end-goal of this entire effort.  It might take a few steps to get there.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":23,"context_line":"  even adding logic such as: Is this router new, or is merely an update?"},{"line_number":24,"context_line":"  Is there a way to decouple this code from the agent, or otherwise make it"},{"line_number":25,"context_line":"  less invasive?"},{"line_number":26,"context_line":"* Should events be sent in dedicated Eventlet threads? I think that if needed,"},{"line_number":27,"context_line":"  this functionality can be added at a later date."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Partially-implements: blueprint restructure-l3-agent"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5a890539_c629bf9c","line":26,"updated":"2014-12-02 00:43:14.000000000","message":"Please no, not yet.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}],"neutron/agent/l3_agent.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":747,"context_line":"                        router\u003drouter,"},{"line_number":748,"context_line":"                        use_ipv6\u003dself.use_ipv6,"},{"line_number":749,"context_line":"                        ns_name\u003dns_name)"},{"line_number":750,"context_line":"        self.driver_manager.notify_observers(\u0027before_router_add\u0027, ri)"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"        self.router_info[router_id] \u003d ri"},{"line_number":753,"context_line":"        if self.conf.use_namespaces:"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_6139d5d7","line":750,"updated":"2014-12-02 00:43:14.000000000","message":"Is there an alternative to using strings here?  What if the DriverManager implemented the same interface as the individual observers?  Then, you could call self.driver_manager.before_router_add(ri).  Would that be weird?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":747,"context_line":"                        router\u003drouter,"},{"line_number":748,"context_line":"                        use_ipv6\u003dself.use_ipv6,"},{"line_number":749,"context_line":"                        ns_name\u003dns_name)"},{"line_number":750,"context_line":"        self.driver_manager.notify_observers(\u0027before_router_add\u0027, ri)"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"        self.router_info[router_id] \u003d ri"},{"line_number":753,"context_line":"        if self.conf.use_namespaces:"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_b1304aea","line":750,"in_reply_to":"5a890539_6139d5d7","updated":"2014-12-02 21:06:35.000000000","message":"Thinking this notifier should be at the same level as the after_router_add.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":1794,"context_line":""},{"line_number":1795,"context_line":"        self._add_and_or_process_router(router)"},{"line_number":1796,"context_line":""},{"line_number":1797,"context_line":"    def _add_and_or_process_router(self, router):"},{"line_number":1798,"context_line":"        new_router \u003d False"},{"line_number":1799,"context_line":"        if router[\u0027id\u0027] not in self.router_info:"},{"line_number":1800,"context_line":"            self._router_added(router[\u0027id\u0027], router)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_a102ddb1","line":1797,"updated":"2014-12-02 00:43:14.000000000","message":"I\u0027d like to refactor this a bit.  Just playing around:\n\n        if router[\u0027id\u0027] not in self.router_info:\n            self._add_and_process_router(router)\n        else:\n            self._update_and_process_router(router)\n\n    def _add_and_process_router(self, router):\n        self._router_added(router[\u0027id\u0027], router) # \u003c-- Inlining this whole method could restore some symmetry to the notifications.\n        ri \u003d self.router_info[router[\u0027id\u0027]]\n        ri.router \u003d router\n        \n        self.process_router(ri)\n        self.driver_manager.notify_observers(\u0027after_router_add\u0027, ri)\n        \n    def _update_and_process_router(self, router):\n        ri \u003d self.router_info[router[\u0027id\u0027]]\n        ri.router \u003d router\n    \n        self.driver_manager.notify_observers(\u0027before_router_update\u0027, ri)\n        self.process_router(ri)\n        self.driver_manager.notify_observers(\u0027after_router_update\u0027, ri)","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":1794,"context_line":""},{"line_number":1795,"context_line":"        self._add_and_or_process_router(router)"},{"line_number":1796,"context_line":""},{"line_number":1797,"context_line":"    def _add_and_or_process_router(self, router):"},{"line_number":1798,"context_line":"        new_router \u003d False"},{"line_number":1799,"context_line":"        if router[\u0027id\u0027] not in self.router_info:"},{"line_number":1800,"context_line":"            self._router_added(router[\u0027id\u0027], router)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_ce28d199","line":1797,"in_reply_to":"5a890539_a102ddb1","updated":"2014-12-02 21:06:35.000000000","message":"That does seem better. The original logic does a lot in one method, and these refinements are clearing things up.  I\u0027m wondering if the notify_observers(\u0027before_router_add\u0027 should be here, instead of in _router_added()?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ec8adc6b81d08343330818b3c710492ef952843a","unresolved":false,"context_lines":[{"line_number":1797,"context_line":"    def _add_and_or_process_router(self, router):"},{"line_number":1798,"context_line":"        new_router \u003d False"},{"line_number":1799,"context_line":"        if router[\u0027id\u0027] not in self.router_info:"},{"line_number":1800,"context_line":"            self._router_added(router[\u0027id\u0027], router)"},{"line_number":1801,"context_line":"            new_router \u003d True"},{"line_number":1802,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"},{"line_number":1803,"context_line":"        ri.router \u003d router"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_d8f6055d","line":1800,"updated":"2014-12-01 19:59:17.000000000","message":"I\u0027m struggling with verifying that the call sequence is the same with the notifications. Here\u0027s how I think (and this is convoluted code) the call sequence was originally for VPN:\n\n    vpn_agent._process_router_if_compatible()\n        super -\u003e l3_agent._process_router_if_compatible()\n            vpn_agent._router_added()\n                super -\u003e l3_agent._router_added()\n                for device... device.create_router()\n            self.process_router()\n        for device... device.sync()\n\nWith L1800, it would call _router_added L742. Now, we wouldn\u0027t want to call before_router_add notify to do VPN steps, as L3 agent _router_add is not finished. After return though, this code will do process_router L1808, before we could call notify for after_router_add on L1811, which is different, no?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":1807,"context_line":""},{"line_number":1808,"context_line":"        self.process_router(ri)"},{"line_number":1809,"context_line":""},{"line_number":1810,"context_line":"        event \u003d \u0027after_router_add\u0027 if new_router else \u0027after_router_update\u0027"},{"line_number":1811,"context_line":"        self.driver_manager.notify_observers(event, ri)"},{"line_number":1812,"context_line":""},{"line_number":1813,"context_line":"    def _process_router_update(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_61c3958a","line":1810,"updated":"2014-12-02 00:43:14.000000000","message":"I understand the desire to avoid verbose code but, to me, this reads better:\n\n  if new_router:\n      self.driver_manager.notify_observers(\u0027...add\u0027, ...)\n  else:\n      self.driver_manager.notify_observers(\u0027...update\u0027, ...)","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":1807,"context_line":""},{"line_number":1808,"context_line":"        self.process_router(ri)"},{"line_number":1809,"context_line":""},{"line_number":1810,"context_line":"        event \u003d \u0027after_router_add\u0027 if new_router else \u0027after_router_update\u0027"},{"line_number":1811,"context_line":"        self.driver_manager.notify_observers(event, ri)"},{"line_number":1812,"context_line":""},{"line_number":1813,"context_line":"    def _process_router_update(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_71418238","line":1810,"in_reply_to":"5a890539_61c3958a","updated":"2014-12-02 21:06:35.000000000","message":"@Carl I like your suggestion on L1797.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}],"neutron/agent/l3_agent_driver_manager.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"2a54d1779ced4e21da8858525358a5524e073a9c","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":30,"context_line":"        for observer in self.observers:"},{"line_number":31,"context_line":"            getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_7d2a9e91","line":31,"updated":"2014-11-27 21:39:10.000000000","message":"We should probably check that the observer has a method for this notification.","commit_id":"ddd534fce5abd98b45e156e4407b4c589e74d786"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"03095e2618841969cd910be8142dec59b5fa5dcc","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":30,"context_line":"        for observer in self.observers:"},{"line_number":31,"context_line":"            getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_6e33ab13","line":31,"in_reply_to":"5a890539_7d2a9e91","updated":"2014-11-29 15:38:40.000000000","message":"Moving this comment to the latest PS so I don\u0027t forget it.","commit_id":"ddd534fce5abd98b45e156e4407b4c589e74d786"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"8b95fbbb1e77cfb10815f773442b062b6bbf3881","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        self.observers.append(observer)"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":30,"context_line":"        for observer in self.observers:"},{"line_number":31,"context_line":"            getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a890539_8e3a97e5","line":30,"updated":"2014-11-29 15:39:46.000000000","message":"On PS2, Kevin Benton wrote:\n\n\u003e We should probably check that the observer has a method for this notification.\n\nI think that implementing the observer methods should be mandatory, that way we get more explicit validation.","commit_id":"591c8a066680b29de03dcd6d712ec6e65f058ab4"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"7586bc475d92eb7aa3e099a9cf403f1173e834d9","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        self.observers.append(observer)"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":30,"context_line":"        for observer in self.observers:"},{"line_number":31,"context_line":"            getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a890539_cead7a9d","line":30,"in_reply_to":"5a890539_8e3a97e5","updated":"2014-11-30 15:05:02.000000000","message":"Looking at the code again, I\u0027d rather add a check here and not complicate things.","commit_id":"591c8a066680b29de03dcd6d712ec6e65f058ab4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"90625e800ab80f66fde86f28c6d4b3fa46979ffa","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        self.observers.append(observer)"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":30,"context_line":"        for observer in self.observers:"},{"line_number":31,"context_line":"            getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a890539_e1c6a515","line":30,"in_reply_to":"5a890539_cead7a9d","updated":"2014-12-02 00:41:23.000000000","message":"Implementing the observer is not only mandatory, it is free.  They all implement the same interface which provides default implementations for each of the notifications.","commit_id":"591c8a066680b29de03dcd6d712ec6e65f058ab4"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    under the License."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"class DriverManager(object):"},{"line_number":18,"context_line":"    def __init__(self):"},{"line_number":19,"context_line":"        self.observers \u003d set()"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_31c7fa99","line":17,"updated":"2014-12-02 21:06:35.000000000","message":"Concerned about the naming here, as driver is very overloaded. Maybe L3EventNotifier, L3ObserverNotifier, L3Publisher (if we rename to publish/subscribe).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ec8adc6b81d08343330818b3c710492ef952843a","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def add_observer(self, observer):"},{"line_number":22,"context_line":"        if observer in self.observers:"},{"line_number":23,"context_line":"            raise TypeError(_(\u0027Observer %s is already registered.\u0027) % observer)"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"        self.observers.add(observer)"},{"line_number":26,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_1d023f09","line":23,"updated":"2014-12-01 19:59:17.000000000","message":"Should this be ignored for the case where an agent fails and is restarted and tries to re-add the same driver?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def add_observer(self, observer):"},{"line_number":22,"context_line":"        if observer in self.observers:"},{"line_number":23,"context_line":"            raise TypeError(_(\u0027Observer %s is already registered.\u0027) % observer)"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"        self.observers.add(observer)"},{"line_number":26,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_e17305b2","line":23,"in_reply_to":"5a890539_1d023f09","updated":"2014-12-02 00:43:14.000000000","message":"If the agent fails then self.observers is reset.  I don\u0027t think we need to worry too much about duplicate registrations.  This is fine.  I wouldn\u0027t care if it didn\u0027t do this check since it is a set and won\u0027t end up with duplicate registrations.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ec8adc6b81d08343330818b3c710492ef952843a","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"        self.observers.add(observer)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":28,"context_line":"        for observer in self.observers:"},{"line_number":29,"context_line":"            if hasattr(observer, notification):"},{"line_number":30,"context_line":"                getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_1d34fff9","line":27,"updated":"2014-12-01 19:59:17.000000000","message":"May be good to have a docstring describing this and the assumptions being made (e.g. error handling).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"de7016ca3bf26aad1cec16103d362cf776de78ea","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":28,"context_line":"        for observer in self.observers:"},{"line_number":29,"context_line":"            if hasattr(observer, notification):"},{"line_number":30,"context_line":"                getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_c15c491c","line":29,"updated":"2014-12-02 00:43:14.000000000","message":"if not hasattr?  Just ignore it?  If they all inherit from the interface then they\u0027ll all have it.  If they don\u0027t have it, then someone has made a mistake in the string naming the notification.  (Strings, yuck).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":28,"context_line":"        for observer in self.observers:"},{"line_number":29,"context_line":"            if hasattr(observer, notification):"},{"line_number":30,"context_line":"                getattr(observer, notification)(*args)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_718982c9","line":29,"in_reply_to":"5a890539_c15c491c","updated":"2014-12-02 21:06:35.000000000","message":"Seems like there are two related aspects here. One is whether to use strings or method names. The other is to try to separate the notifier (this class) from the observer (the Service classes).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}],"neutron/services/advanced_service.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ec8adc6b81d08343330818b3c710492ef952843a","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    _instance \u003d None"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def __init__(self):"},{"line_number":28,"context_line":"        \"\"\"Use cls.instance instead of initializing an instance directly."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"        For example:"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_7840b135","line":27,"updated":"2014-12-01 19:59:17.000000000","message":"Must we have a singleton for the Service classes? There should only be one agent for each type of service. Just wondering if this is needed.\n\nCould just initialize self.l3_agent as part of __init__() in this base class.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"fc83578021baa7065ccf16e795c0e14a34a5bdfe","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    _instance \u003d None"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def __init__(self):"},{"line_number":28,"context_line":"        \"\"\"Use cls.instance instead of initializing an instance directly."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"        For example:"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_d2940239","line":27,"in_reply_to":"5a890539_7840b135","updated":"2014-12-02 21:06:35.000000000","message":"After talking to Assaf, I see the need for the singleton - in case out of tree services, so could have multiple VPN agents registering for VPN notifications, for example.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e000889474bb9e5be637edfc817d16a0b13a02dd","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \"\"\""},{"line_number":40,"context_line":"        self.l3_agent \u003d None"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def register(self, l3_agent, driver_manager):"},{"line_number":43,"context_line":"        self.l3_agent \u003d l3_agent"},{"line_number":44,"context_line":"        driver_manager.add_observer(self)"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_1a38ea9e","line":42,"updated":"2014-12-05 01:55:33.000000000","message":"Some questions... we\u0027re passing in the manager, and an instance is created by the agent. This implies that there will be a set of observers for each service type. Do we want that, or a single set?\n\nIf single, do we want to have one driver manager? A singleton? Or just have a add_observer() class method?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"9617b257a7d092ad9e85d41e38f7d5b7d96b8035","unresolved":false,"context_lines":[{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @classmethod"},{"line_number":47,"context_line":"    def instance(cls):"},{"line_number":48,"context_line":"        if not cls._instance:"},{"line_number":49,"context_line":"            with lockutils.lock(\u0027instance\u0027):"},{"line_number":50,"context_line":"                if not cls._instance:"},{"line_number":51,"context_line":"                    cls._instance \u003d cls()"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_5a06f21c","line":48,"updated":"2014-12-05 01:35:38.000000000","message":"Are there times you would call instance() other than when you want to create the service and register it?\n\nIf so, I\u0027m not sure that I like that this method will create the instance automatically.  What if some code somewhere calls instance() somehow before the code which would call register(...) immediately afterward.  It gets a newly created instance that isn\u0027t registered.  Kind of a weird state.\n\nIf not, why not combine this and register in to one method?  It doesn\u0027t make much sense to me to create one without registering it.\n\nI guess I would vote to combine instance and register.  Then, if access to the current instance is needed in another place, another method could be added (and maybe throw an exception if the instance hasn\u0027t been created).","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e000889474bb9e5be637edfc817d16a0b13a02dd","unresolved":false,"context_lines":[{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @classmethod"},{"line_number":47,"context_line":"    def instance(cls):"},{"line_number":48,"context_line":"        if not cls._instance:"},{"line_number":49,"context_line":"            with lockutils.lock(\u0027instance\u0027):"},{"line_number":50,"context_line":"                if not cls._instance:"},{"line_number":51,"context_line":"                    cls._instance \u003d cls()"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_1a9c6abf","line":48,"in_reply_to":"5a890539_5a06f21c","updated":"2014-12-05 01:55:33.000000000","message":"Yeah, as we discussed on IM. I\u0027m thinking that register can be done as part of instance().","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}],"neutron/services/firewall/agents/firewall_service.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"8b95fbbb1e77cfb10815f773442b062b6bbf3881","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class FirewallService(advanced_service.AdvancedService):"},{"line_number":26,"context_line":"    \"\"\"Firewall Service observer.\"\"\""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def __init__(self, l3_agent, driver_manager):"},{"line_number":29,"context_line":"        super(FirewallService, self).__init__(l3_agent, driver_manager)"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5a890539_2e29a39b","line":27,"updated":"2014-11-29 15:39:46.000000000","message":"This method can be removed.","commit_id":"591c8a066680b29de03dcd6d712ec6e65f058ab4"}],"neutron/services/firewall/agents/l3reference/firewall_l3_agent.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e000889474bb9e5be637edfc817d16a0b13a02dd","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                                             conf.host)"},{"line_number":82,"context_line":"        super(FWaaSL3AgentRpcCallback, self).__init__(host\u003dconf.host)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def add_observer(self, observer):"},{"line_number":85,"context_line":"        \"\"\"Services that will get notified for events.\"\"\""},{"line_number":86,"context_line":"        # TODO(pcm): Because of the current class hierarchy for agents, this"},{"line_number":87,"context_line":"        # has to be here, instead of in L3NATAgent, so that FW agent can"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_9a721a7a","line":84,"updated":"2014-12-05 01:55:33.000000000","message":"This should go, as have separate class for adding observers.","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}],"neutron/services/vpn/vpn_service.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"2a54d1779ced4e21da8858525358a5524e073a9c","unresolved":false,"context_lines":[{"line_number":26,"context_line":"class VPNService(advanced_service.AdvancedService):"},{"line_number":27,"context_line":"    \"\"\"VPN Service observer.\"\"\""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def __init__(self, l3_agent, driver_manager):"},{"line_number":30,"context_line":"        super(VPNService, self).__init__(l3_agent, driver_manager)"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def load_device_drivers(self, agent, host):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_5d2de2a7","line":29,"updated":"2014-11-27 21:39:10.000000000","message":"This __init__ method doesn\u0027t do anything. It can go away","commit_id":"ddd534fce5abd98b45e156e4407b4c589e74d786"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"8b95fbbb1e77cfb10815f773442b062b6bbf3881","unresolved":false,"context_lines":[{"line_number":26,"context_line":"class VPNService(advanced_service.AdvancedService):"},{"line_number":27,"context_line":"    \"\"\"VPN Service observer.\"\"\""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def __init__(self, l3_agent, driver_manager):"},{"line_number":30,"context_line":"        super(VPNService, self).__init__(l3_agent, driver_manager)"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def load_device_drivers(self, agent, host):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a890539_0e2ea7a5","line":29,"updated":"2014-11-29 15:39:46.000000000","message":"This method can be removed.","commit_id":"591c8a066680b29de03dcd6d712ec6e65f058ab4"}],"neutron/tests/unit/services/firewall/agents/l3reference/test_firewall_l3_agent.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e000889474bb9e5be637edfc817d16a0b13a02dd","unresolved":false,"context_lines":[{"line_number":46,"context_line":"        neutron_service_plugins \u003d service_plugins"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"        def __init__(self, conf):"},{"line_number":49,"context_line":"            self.driver_manager \u003d mock.Mock()"},{"line_number":50,"context_line":"            super(FWaasTestAgent, self).__init__(conf)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    return FWaasTestAgent"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_1a5f0ae6","line":49,"updated":"2014-12-05 01:55:33.000000000","message":"Just noting, that, because this is a mock agent, it doesn\u0027t really test that the real FWaaS agent has a driver_manager field.  Not sure how to correct that. Mock the real agent?","commit_id":"c5c6f783c33267ef9511c13b07cf83fd0ea5fbc1"}]}