)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":25,"context_line":"TODO:"},{"line_number":26,"context_line":"- Add unit tests for new methods created"},{"line_number":27,"context_line":"- Create concrete service observers for FWaaS, LBaaS, metadata proxy."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"The next few steps are perceived to be:"},{"line_number":30,"context_line":"- Define the notification handlers in the observer base class."},{"line_number":31,"context_line":"- Move handlers from the existing agents into concrete observer handlers."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5a890539_e083d653","line":28,"updated":"2014-11-23 10:14:23.000000000","message":"Paul wrote:\n\u003e Should I pull the add_observer/notify_observer logic from this patch set, as it will be the focus of a future refactoring, or should I leave it in with gratuitous comments that it is only an example?\n\nI agree that beefing up registration so that the agent doesn\u0027t have to know about its drivers should be handled in a follow up patch, however I think this patch should widen its scope so that it\u0027s the basis of a system where follow up patches only move drivers to the new system. I don\u0027t think there\u0027s a lot of work to get there: One concern is exception handling and making sure that all drivers get a notification even if one fails, but that you fail the grand operation.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":25,"context_line":"TODO:"},{"line_number":26,"context_line":"- Add unit tests for new methods created"},{"line_number":27,"context_line":"- Create concrete service observers for FWaaS, LBaaS, metadata proxy."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"The next few steps are perceived to be:"},{"line_number":30,"context_line":"- Define the notification handlers in the observer base class."},{"line_number":31,"context_line":"- Move handlers from the existing agents into concrete observer handlers."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5a890539_c0a41a4d","line":28,"in_reply_to":"5a890539_e083d653","updated":"2014-11-23 13:29:18.000000000","message":"On the ML, I published some notes on the steps that Carl and I were thinking about, which are summarized on L30-32.\n\nMy goal in this refactoring was to really focus on the observer hierarchy. As part of that, I did need to define the add_observer(), and I wanted to also give a feel for where the second refactoring was going with a really simple example of how it could be hooked up.\n\nI\u0027m thinking now, that, it probably wasn\u0027t a good idea, and is likely muddying this refactoring.\n\nShould I remove the notification logic entirely, and just leave an empty add_observer() method?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d94c87c2ec386f6b230e11d9860fad1249cee30c","unresolved":false,"context_lines":[{"line_number":22,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":23,"context_line":"  changes to allow existing code to continue to work."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"TODO:"},{"line_number":26,"context_line":"- Loadbalancer must be added. Metadata proxy too?"},{"line_number":27,"context_line":"- Need class for Subject (containing add_observer). Location?"},{"line_number":28,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"5a890539_e694fbe9","line":25,"updated":"2014-12-02 00:44:56.000000000","message":"I don\u0027t think we want TODO in the commit message, do we?","commit_id":"98b6725bdd9a1491d99da0f26c45c2e633a62c36"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"dfe112557dca71695da72fa882e1cf891740c24a","unresolved":false,"context_lines":[{"line_number":22,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":23,"context_line":"  changes to allow existing code to continue to work."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"TODO:"},{"line_number":26,"context_line":"- Loadbalancer must be added. Metadata proxy too?"},{"line_number":27,"context_line":"- Need class for Subject (containing add_observer). Location?"},{"line_number":28,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"5a890539_f482c050","line":25,"in_reply_to":"5a890539_e694fbe9","updated":"2014-12-02 21:12:52.000000000","message":"No. This is still a WIP, and I was indicating the pieces still needed to be completed.\n\nBTW, I talked to Assaf, and the plan is to merge in the other change set into this one.","commit_id":"98b6725bdd9a1491d99da0f26c45c2e633a62c36"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a308421efac00139ea8bb2ca4220e89d1574fcab","unresolved":false,"context_lines":[{"line_number":24,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":25,"context_line":"  changes to allow existing code to continue to work."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"This WIP still needs to add loadbalancer and metadata proxy observers."},{"line_number":28,"context_line":"There are two questions in the patch set (as TODO), requesting feedback"},{"line_number":29,"context_line":"on whether we should \"try\" to prevent instances of AdvancedService base"},{"line_number":30,"context_line":"class, and whether there should be one observer list for all services."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5a890539_603bd8c8","line":27,"updated":"2014-12-05 21:42:31.000000000","message":"Q: for LB team. Is the reference LB agent independent of the L3 agent hierarchy? I did some digging, and it seems like it is separate (and hence can be ignored in this process). Is that true?","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":24,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":25,"context_line":"  changes to allow existing code to continue to work."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"This WIP still needs to add loadbalancer and metadata proxy observers."},{"line_number":28,"context_line":"There are two questions in the patch set (as TODO), requesting feedback"},{"line_number":29,"context_line":"on whether we should \"try\" to prevent instances of AdvancedService base"},{"line_number":30,"context_line":"class, and whether there should be one observer list for all services."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5a890539_1aa0c090","line":27,"in_reply_to":"5a890539_603bd8c8","updated":"2014-12-06 00:49:50.000000000","message":"Talked to Doug W, and the Loadbalance driver IS independent, so nothing needed there. Assaf has been doing metadata proxy, and has a commit out. Probably most prudent for Assaf to address that under that commit, and not add that here. Will verify with Assaf.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"8ef7f1697672ff369a3924dab68c778edcc0db03","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Please keep these things in mind, when reviewing:"},{"line_number":22,"context_line":"- The goal is to refactor the code and not \u0027improve/change\u0027 functionality."},{"line_number":23,"context_line":"- This is one step of a series of steps to move to the \u0027final\u0027 restructuring."},{"line_number":24,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":25,"context_line":"  changes to allow existing code to continue to work."},{"line_number":26,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3a961159_5c587e56","line":23,"updated":"2014-12-08 22:52:27.000000000","message":"I need to wrap these long lines.","commit_id":"783340335380dcc490b8f031b477725ce85ed11b"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"8ef7f1697672ff369a3924dab68c778edcc0db03","unresolved":false,"context_lines":[{"line_number":24,"context_line":"- Because we\u0027re incrementally changing the code, there may be temporary"},{"line_number":25,"context_line":"  changes to allow existing code to continue to work."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"There are two questions in the patch set (as TODO), requesting feedback"},{"line_number":28,"context_line":"on whether we should \"try\" to prevent instances of AdvancedService base"},{"line_number":29,"context_line":"class, and whether there should be one observer list for all services."},{"line_number":30,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3a961159_dc3b8ee9","line":27,"updated":"2014-12-08 22:52:27.000000000","message":"Need to decide on these...","commit_id":"783340335380dcc490b8f031b477725ce85ed11b"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"38fc51380ac8ca89ecabb0aaef59fbff099ee277","unresolved":false,"context_lines":[{"line_number":26,"context_line":"- Because we\u0027re incrementally changing the code, there may be"},{"line_number":27,"context_line":"  temporary changes to allow existing code to continue to work."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Co-Authored-By: Assaf Muller"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Change-Id: I674c72e37b56aa1f729110310e6f697297c47c09"},{"line_number":32,"context_line":"Partially-implements: blueprint restructure-l3-agent"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3a961159_b0588533","line":29,"updated":"2014-12-09 18:48:30.000000000","message":"This should have the same format as the Author line above.  Include Assaf\u0027s email address like this:\n\n  Co-Authored-By:     Assaf Muller \u003camuller@redhat.com\u003e","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"2da3886a25b430a032639b63dd1fbce8d3f9f352","unresolved":false,"context_lines":[{"line_number":26,"context_line":"- Because we\u0027re incrementally changing the code, there may be"},{"line_number":27,"context_line":"  temporary changes to allow existing code to continue to work."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Co-Authored-By: Assaf Muller"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Change-Id: I674c72e37b56aa1f729110310e6f697297c47c09"},{"line_number":32,"context_line":"Partially-implements: blueprint restructure-l3-agent"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"3a961159_9f3ab5cb","line":29,"in_reply_to":"3a961159_b0588533","updated":"2014-12-09 19:18:49.000000000","message":"Done","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"}],"neutron/agent/l3/agent.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"846eb06c02b03a1260732b3443e6d1628e4e2034","unresolved":false,"context_lines":[{"line_number":1529,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"},{"line_number":1530,"context_line":"        ri.router \u003d router"},{"line_number":1531,"context_line":"        self.event_observers.notify("},{"line_number":1532,"context_line":"            adv_svc.AdvancedService.before_process_router_for_add, ri)"},{"line_number":1533,"context_line":"        self.process_router(ri)"},{"line_number":1534,"context_line":"        self.event_observers.notify("},{"line_number":1535,"context_line":"            adv_svc.AdvancedService.after_process_router_for_add, ri)"}],"source_content_type":"text/x-python","patch_set":12,"id":"3a961159_8fb20e4a","line":1532,"updated":"2014-12-12 18:23:21.000000000","message":"I think after_router_remove, before_router_remove, and these two should be collapsed in to two notifications.  I see that you want to preserve symmetry and there is concern about where metadata proxy will fit.  However, I think it is more important to define notifications that make sense from an external point of view.  I think it would be okay to just remove the two unused notifications and rename after_process_router_for_add to after_router_added.\n\nFuture refactoring can be done to bring symmetry back.","commit_id":"61d6fe6d0fae9cb66e370416730d771a67af3765"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"846eb06c02b03a1260732b3443e6d1628e4e2034","unresolved":false,"context_lines":[{"line_number":1538,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"},{"line_number":1539,"context_line":"        ri.router \u003d router"},{"line_number":1540,"context_line":"        self.event_observers.notify("},{"line_number":1541,"context_line":"            adv_svc.AdvancedService.before_process_router_for_update, ri)"},{"line_number":1542,"context_line":"        self.process_router(ri)"},{"line_number":1543,"context_line":"        self.event_observers.notify("},{"line_number":1544,"context_line":"            adv_svc.AdvancedService.after_process_router_for_update, ri)"}],"source_content_type":"text/x-python","patch_set":12,"id":"3a961159_6f38823d","line":1541,"updated":"2014-12-12 18:23:21.000000000","message":"These should be renamed to be consistent with the other notifications.","commit_id":"61d6fe6d0fae9cb66e370416730d771a67af3765"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a948076459ccbd2abdaaa1535a9c84d2beb14058","unresolved":false,"context_lines":[{"line_number":1528,"context_line":"        ri.router \u003d router"},{"line_number":1529,"context_line":"        self.process_router(ri)"},{"line_number":1530,"context_line":"        self.event_observers.notify("},{"line_number":1531,"context_line":"            adv_svc.AdvancedService.after_router_added, ri)"},{"line_number":1532,"context_line":""},{"line_number":1533,"context_line":"    def _process_updated_router(self, router):"},{"line_number":1534,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_e51491ea","line":1531,"updated":"2014-12-12 18:56:19.000000000","message":"NOTE:  It looks a bit unsymmetrical to fire this notification here and the corresponding \"before\" notification up on L466.  However, I\u0027m okay with this as an intermediate step.  I\u0027d prefer to keep the notifications simple and symmetrical from the observers point of view.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"1167244e04fb6e8d7f9a238126adf9e4333b495b","unresolved":false,"context_lines":[{"line_number":1528,"context_line":"        ri.router \u003d router"},{"line_number":1529,"context_line":"        self.process_router(ri)"},{"line_number":1530,"context_line":"        self.event_observers.notify("},{"line_number":1531,"context_line":"            adv_svc.AdvancedService.after_router_added, ri)"},{"line_number":1532,"context_line":""},{"line_number":1533,"context_line":"    def _process_updated_router(self, router):"},{"line_number":1534,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_ef438b69","line":1531,"in_reply_to":"3a961159_4ff65f90","updated":"2014-12-12 21:26:02.000000000","message":"+1","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":1528,"context_line":"        ri.router \u003d router"},{"line_number":1529,"context_line":"        self.process_router(ri)"},{"line_number":1530,"context_line":"        self.event_observers.notify("},{"line_number":1531,"context_line":"            adv_svc.AdvancedService.after_router_added, ri)"},{"line_number":1532,"context_line":""},{"line_number":1533,"context_line":"    def _process_updated_router(self, router):"},{"line_number":1534,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_4ff65f90","line":1531,"in_reply_to":"3a961159_a9bd03da","updated":"2014-12-12 21:13:46.000000000","message":"Yeah, discussions were heading in the direction of reducing the number of notifications. However, I\u0027d like to see us get the commit in, and then follow up with another commit to address this, as the VPN and FW companion commits need the AdvancedService base class that we have in this commit. OK?","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"4f1a1526f7bd2c864b27bd827e23e47d335e4a33","unresolved":false,"context_lines":[{"line_number":1528,"context_line":"        ri.router \u003d router"},{"line_number":1529,"context_line":"        self.process_router(ri)"},{"line_number":1530,"context_line":"        self.event_observers.notify("},{"line_number":1531,"context_line":"            adv_svc.AdvancedService.after_router_added, ri)"},{"line_number":1532,"context_line":""},{"line_number":1533,"context_line":"    def _process_updated_router(self, router):"},{"line_number":1534,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_a9bd03da","line":1531,"in_reply_to":"3a961159_e51491ea","updated":"2014-12-12 20:04:59.000000000","message":"I agree, and I think that we can shuffle the agent methods around a bit so that the events are symmetrical in a later patch.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a948076459ccbd2abdaaa1535a9c84d2beb14058","unresolved":false,"context_lines":[{"line_number":1539,"context_line":"        self.event_observers.notify("},{"line_number":1540,"context_line":"            adv_svc.AdvancedService.after_router_updated, ri)"},{"line_number":1541,"context_line":""},{"line_number":1542,"context_line":"    def _process_router_update(self):"},{"line_number":1543,"context_line":"        for rp, update in self._queue.each_update_to_next_router():"},{"line_number":1544,"context_line":"            LOG.debug(\"Starting router update for %s\", update.id)"},{"line_number":1545,"context_line":"            router \u003d update.router"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_a5cc498a","line":1542,"updated":"2014-12-12 18:56:19.000000000","message":"It is odd that we just added \"_process_update_router\" when we already have a \"_process_router_update\".  This could be confusing.  Should we change the names of the previous two methods to something more unique?","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"afde848c9aabda34961b2410fa0cc10bfb8b10ab","unresolved":false,"context_lines":[{"line_number":1539,"context_line":"        self.event_observers.notify("},{"line_number":1540,"context_line":"            adv_svc.AdvancedService.after_router_updated, ri)"},{"line_number":1541,"context_line":""},{"line_number":1542,"context_line":"    def _process_router_update(self):"},{"line_number":1543,"context_line":"        for rp, update in self._queue.each_update_to_next_router():"},{"line_number":1544,"context_line":"            LOG.debug(\"Starting router update for %s\", update.id)"},{"line_number":1545,"context_line":"            router \u003d update.router"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_4fd9fff1","line":1542,"in_reply_to":"3a961159_a5cc498a","updated":"2014-12-12 21:26:48.000000000","message":"+1, existing l3-agent method names are already enough unclear (sick)","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"6f08eb532d455eea5cc3fd1f3bf6c060b2e1f4e5","unresolved":false,"context_lines":[{"line_number":1539,"context_line":"        self.event_observers.notify("},{"line_number":1540,"context_line":"            adv_svc.AdvancedService.after_router_updated, ri)"},{"line_number":1541,"context_line":""},{"line_number":1542,"context_line":"    def _process_router_update(self):"},{"line_number":1543,"context_line":"        for rp, update in self._queue.each_update_to_next_router():"},{"line_number":1544,"context_line":"            LOG.debug(\"Starting router update for %s\", update.id)"},{"line_number":1545,"context_line":"            router \u003d update.router"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_fa7ee768","line":1542,"in_reply_to":"3a961159_a5cc498a","updated":"2014-12-12 21:29:17.000000000","message":"Per IRC discussion, we can change this in the next refactor, as we plan on reworking L1520-1540","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"}],"neutron/agent/l3/event_observer.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"7d9113a60e9c9b7ea28306b644d1b0fe09419847","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2014 OpenStack Foundation"},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":8,"id":"3a961159_8b029b3c","line":1,"updated":"2014-12-09 01:27:59.000000000","message":"File name is wrong. Fixing...","commit_id":"783340335380dcc490b8f031b477725ce85ed11b"}],"neutron/agent/l3/event_observers.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"38fc51380ac8ca89ecabb0aaef59fbff099ee277","unresolved":false,"context_lines":[{"line_number":25,"context_line":"        \"\"\"Add a listener for L3 agent notifications.\"\"\""},{"line_number":26,"context_line":"        # TODO(pcm): Should we make add() and notify() class methods, and"},{"line_number":27,"context_line":"        # self.observers a class attribute? This would share the observers"},{"line_number":28,"context_line":"        # across all services (versus each service having its own set), and"},{"line_number":29,"context_line":"        # services would not need to pass in (or use the default)"},{"line_number":30,"context_line":"        # L3EventObserver objects. See instance() in advanced_services.py."},{"line_number":31,"context_line":"        self.observers.add(observer)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_10feb938","line":28,"updated":"2014-12-09 18:48:30.000000000","message":"It is not true that each service has its own set.  There is one instance of this class instantiated in the agent.  I think this whole TODO can be removed.  I don\u0027t think we should make these class methods.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"2da3886a25b430a032639b63dd1fbce8d3f9f352","unresolved":false,"context_lines":[{"line_number":25,"context_line":"        \"\"\"Add a listener for L3 agent notifications.\"\"\""},{"line_number":26,"context_line":"        # TODO(pcm): Should we make add() and notify() class methods, and"},{"line_number":27,"context_line":"        # self.observers a class attribute? This would share the observers"},{"line_number":28,"context_line":"        # across all services (versus each service having its own set), and"},{"line_number":29,"context_line":"        # services would not need to pass in (or use the default)"},{"line_number":30,"context_line":"        # L3EventObserver objects. See instance() in advanced_services.py."},{"line_number":31,"context_line":"        self.observers.add(observer)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_bf3cd1c0","line":28,"in_reply_to":"3a961159_10feb938","updated":"2014-12-09 19:18:49.000000000","message":"Done","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"97ed11441a8a4a26dda07ef62d7b616558a2220f","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        # self.observers a class attribute? This would share the observers"},{"line_number":28,"context_line":"        # across all services (versus each service having its own set), and"},{"line_number":29,"context_line":"        # services would not need to pass in (or use the default)"},{"line_number":30,"context_line":"        # L3EventObserver objects. See instance() in advanced_services.py."},{"line_number":31,"context_line":"        self.observers.add(observer)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_b05b2586","line":30,"updated":"2014-12-09 17:58:54.000000000","message":"nit: advanced_service.py","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ed5bf1424d27f5735e517805766ae77f6b3a7146","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        # self.observers a class attribute? This would share the observers"},{"line_number":28,"context_line":"        # across all services (versus each service having its own set), and"},{"line_number":29,"context_line":"        # services would not need to pass in (or use the default)"},{"line_number":30,"context_line":"        # L3EventObserver objects. See instance() in advanced_services.py."},{"line_number":31,"context_line":"        self.observers.add(observer)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_392ac17e","line":30,"in_reply_to":"3a961159_b05b2586","updated":"2014-12-09 18:36:24.000000000","message":"Done","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e471e3482c9999aaa627ee7b281378d8598a1192","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        is currently done."},{"line_number":39,"context_line":"        \"\"\""},{"line_number":40,"context_line":"        for observer in self.observers:"},{"line_number":41,"context_line":"            l3_event_action(observer, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_e03cf5c1","line":41,"updated":"2014-12-09 22:16:38.000000000","message":"This doesn\u0027t call the right method (not polymorphic).\n\n        notification \u003d l3_event_action.__name__\n        for observer in self.observers:\n            method \u003d getattr(observer, notification)\n            method(*args, **kwargs)","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"9b50d294a43af92d2cd76f80eafd7fcd2c80a6b7","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        is currently done."},{"line_number":39,"context_line":"        \"\"\""},{"line_number":40,"context_line":"        for observer in self.observers:"},{"line_number":41,"context_line":"            l3_event_action(observer, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_68a467a6","line":41,"in_reply_to":"3a961159_e03cf5c1","updated":"2014-12-10 04:13:46.000000000","message":"Good catch Carl! In addition to fixing this, I\u0027ll add UT coverage.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a948076459ccbd2abdaaa1535a9c84d2beb14058","unresolved":false,"context_lines":[{"line_number":19,"context_line":"    \"\"\"Manages observers for L3 agent events.\"\"\""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def __init__(self):"},{"line_number":22,"context_line":"        self.observers \u003d set()"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"    def add(self, observer):"},{"line_number":25,"context_line":"        \"\"\"Add a listener for L3 agent notifications.\"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_65850106","line":22,"updated":"2014-12-12 18:56:19.000000000","message":"Should order matter?  With a set, we are making the assumption that the order of the notifications does not matter.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"4f1a1526f7bd2c864b27bd827e23e47d335e4a33","unresolved":false,"context_lines":[{"line_number":19,"context_line":"    \"\"\"Manages observers for L3 agent events.\"\"\""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def __init__(self):"},{"line_number":22,"context_line":"        self.observers \u003d set()"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"    def add(self, observer):"},{"line_number":25,"context_line":"        \"\"\"Add a listener for L3 agent notifications.\"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_c97a4f87","line":22,"in_reply_to":"3a961159_65850106","updated":"2014-12-12 20:04:59.000000000","message":"This is nice and simple. My take is that if in some point in the future order will matter, we could easily change the code.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":19,"context_line":"    \"\"\"Manages observers for L3 agent events.\"\"\""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def __init__(self):"},{"line_number":22,"context_line":"        self.observers \u003d set()"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"    def add(self, observer):"},{"line_number":25,"context_line":"        \"\"\"Add a listener for L3 agent notifications.\"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_cc05fd11","line":22,"in_reply_to":"3a961159_c97a4f87","updated":"2014-12-12 21:13:46.000000000","message":"Agree.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"4f1a1526f7bd2c864b27bd827e23e47d335e4a33","unresolved":false,"context_lines":[{"line_number":28,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"},{"line_number":29,"context_line":"        \"\"\"Give interested parties a chance to act on event."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        NOTE: If one of the notifications raises an exception, it will"},{"line_number":32,"context_line":"        propagate up, preventing other notifications. This is the same as"},{"line_number":33,"context_line":"        is currently done."},{"line_number":34,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_49381fd3","line":31,"updated":"2014-12-12 20:04:59.000000000","message":"This comment won\u0027t make sense in a couple of months.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":28,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"},{"line_number":29,"context_line":"        \"\"\"Give interested parties a chance to act on event."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        NOTE: If one of the notifications raises an exception, it will"},{"line_number":32,"context_line":"        propagate up, preventing other notifications. This is the same as"},{"line_number":33,"context_line":"        is currently done."},{"line_number":34,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_cc533d15","line":31,"in_reply_to":"3a961159_49381fd3","updated":"2014-12-12 21:13:46.000000000","message":"Should I add a TODO at the end to say to update, later, when things are refactored more?","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"1167244e04fb6e8d7f9a238126adf9e4333b495b","unresolved":false,"context_lines":[{"line_number":28,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"},{"line_number":29,"context_line":"        \"\"\"Give interested parties a chance to act on event."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        NOTE: If one of the notifications raises an exception, it will"},{"line_number":32,"context_line":"        propagate up, preventing other notifications. This is the same as"},{"line_number":33,"context_line":"        is currently done."},{"line_number":34,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_cf37affd","line":31,"in_reply_to":"3a961159_cc533d15","updated":"2014-12-12 21:26:02.000000000","message":"Or, you could just remove it.  We\u0027ve already established that this will preserve existing behavior.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"afde848c9aabda34961b2410fa0cc10bfb8b10ab","unresolved":false,"context_lines":[{"line_number":28,"context_line":"    def notify(self, l3_event_action, *args, **kwargs):"},{"line_number":29,"context_line":"        \"\"\"Give interested parties a chance to act on event."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        NOTE: If one of the notifications raises an exception, it will"},{"line_number":32,"context_line":"        propagate up, preventing other notifications. This is the same as"},{"line_number":33,"context_line":"        is currently done."},{"line_number":34,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_efb54b7e","line":31,"in_reply_to":"3a961159_cc533d15","updated":"2014-12-12 21:26:48.000000000","message":"TODO comments are usually used for such case in neutron code","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":13380,"name":"Sridhar Ramaswamy","email":"srics.r@gmail.com","username":"srics-r"},"change_message_id":"a6dd477c33538ce3029a2c8a6be3b8036239733d","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        \"\"\""},{"line_number":33,"context_line":"        method_name \u003d l3_event_action.__name__"},{"line_number":34,"context_line":"        for observer in self.observers:"},{"line_number":35,"context_line":"            getattr(observer, method_name)(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3a961159_98b852e7","line":35,"updated":"2014-12-16 02:08:57.000000000","message":"Just for my understanding. What is the guidance given to the advanced services handlers here ? What would happen if one of these methods blocks ? Is it safe to assume the behavior will be same as if inheritance were used.","commit_id":"d264666bba0d633f2c6be423bf9f7e08db4ff289"},{"author":{"_account_id":13380,"name":"Sridhar Ramaswamy","email":"srics.r@gmail.com","username":"srics-r"},"change_message_id":"22c6582f7a9ac38a09f4418fdfcf0e2d39b6e83f","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        \"\"\""},{"line_number":33,"context_line":"        method_name \u003d l3_event_action.__name__"},{"line_number":34,"context_line":"        for observer in self.observers:"},{"line_number":35,"context_line":"            getattr(observer, method_name)(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3a961159_01469123","line":35,"in_reply_to":"3a961159_0fab2cd7","updated":"2014-12-16 20:16:20.000000000","message":"Sounds good. \n\n+1 on the reducing the number of handlers. It would confuse the advanced-services developer. We can always add back some of the handlers if there is a genuine need.","commit_id":"d264666bba0d633f2c6be423bf9f7e08db4ff289"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"05a2e5f7ac69a8a4236058d9dad1a9cb460c97d2","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        \"\"\""},{"line_number":33,"context_line":"        method_name \u003d l3_event_action.__name__"},{"line_number":34,"context_line":"        for observer in self.observers:"},{"line_number":35,"context_line":"            getattr(observer, method_name)(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3a961159_0fab2cd7","line":35,"in_reply_to":"3a961159_98b852e7","updated":"2014-12-16 12:49:03.000000000","message":"For this initial restructuring, the plan is to move the logic from the agents (pretty much as-is) to the handler methods in the new service classes. So, the expectation is that the behavior will be the same as it is today (e.g. if an exception is raised, it\u0027ll propagate up). You can look at https://review.openstack.org/140918 for an example of VPN handlers.\n\nNote: Subsequent refactorings may rename some of the handlers, as we\u0027re trying to reduce the number of handlers and it appears that some notification points can be combined to simplify things.","commit_id":"d264666bba0d633f2c6be423bf9f7e08db4ff289"}],"neutron/agent/l3_agent.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":560,"context_line":""},{"line_number":561,"context_line":"        self.target_ex_net_id \u003d None"},{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_00f0e2fc","line":563,"updated":"2014-11-23 10:14:23.000000000","message":"I know that add_observer will be scrapped in the future, but notify_observers should never live in the L3 agent class. One alternative is that we create a new class and the agent will access it via composition. The new class should have unit tests for notify_observers (Covering exception handling, what happens if one driver fails, etc).","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":560,"context_line":""},{"line_number":561,"context_line":"        self.target_ex_net_id \u003d None"},{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_0051c235","line":563,"in_reply_to":"5a890539_00f0e2fc","updated":"2014-11-23 13:29:18.000000000","message":"Just so I understand your point, why do you think the notify_observer should never live in the L3 agent? I was thinking that it would, as an initial step.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"90772da8ac449d87318923cf9c86d830206ddb0d","unresolved":false,"context_lines":[{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_b57bdbbc","line":565,"updated":"2014-11-22 04:34:24.000000000","message":"what if the same observer accidentally registered twice?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_c05c1a1c","line":565,"in_reply_to":"5a890539_4b013369","updated":"2014-11-23 10:14:23.000000000","message":"I would actually keep it a list and raise an exception if a driver registers twice. That shouldn\u0027t happen and signals a logical error in new code.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"659f3bc3b894174e6ec0ce69710931484dab59c3","unresolved":false,"context_lines":[{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_148f4c2b","line":565,"in_reply_to":"5a890539_4b013369","updated":"2014-12-02 00:44:01.000000000","message":"meh\n\nPlease don\u0027t get too complicated.  The service doesn\u0027t run in a separate process or even a separate thread at this point.  So, it can\u0027t die or be killed.  If the service wants to spawn its own process then it will have to take care of the details of doing that.  If we want to make this a generic capability for all services then we can do that later.\n\nI think if a service registers twice, who are we to say it doesn\u0027t want to be notified twice?  I\u0027m not really concerned about this.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c733df361a2e8b3dfdf06455fe8f7a39f442d43b","unresolved":false,"context_lines":[{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_4b013369","line":565,"in_reply_to":"5a890539_b57bdbbc","updated":"2014-11-22 15:38:11.000000000","message":"Good point, I should make a dict, in case a observer crashes and is restarted. Will change on next patch.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":562,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def add_observer(self, observer):"},{"line_number":565,"context_line":"        self.observers.append(observer)"},{"line_number":566,"context_line":"        LOG.debug(\u0027TEMP: Added observer %r\u0027, observer)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_6078c6c4","line":565,"in_reply_to":"5a890539_c05c1a1c","updated":"2014-11-23 13:29:18.000000000","message":"I think a service could be killed or die, and then be restarted, causing it to register a second time, no?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"90772da8ac449d87318923cf9c86d830206ddb0d","unresolved":false,"context_lines":[{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":569,"context_line":"        for observer in self.observers:"},{"line_number":570,"context_line":"            getattr(observer, notification)(*args)"},{"line_number":571,"context_line":""},{"line_number":572,"context_line":"    def _fip_ns_subscribe(self, router_id):"},{"line_number":573,"context_line":"        is_first \u003d (len(self.fip_ns_subscribers) \u003d\u003d 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_d54b0785","line":570,"updated":"2014-11-22 04:34:24.000000000","message":"if one fails the, the rest of the chain goes belly up; also this might be useful to do it in separate threads.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":569,"context_line":"        for observer in self.observers:"},{"line_number":570,"context_line":"            getattr(observer, notification)(*args)"},{"line_number":571,"context_line":""},{"line_number":572,"context_line":"    def _fip_ns_subscribe(self, router_id):"},{"line_number":573,"context_line":"        is_first \u003d (len(self.fip_ns_subscribers) \u003d\u003d 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_40474a77","line":570,"in_reply_to":"5a890539_20a31e01","updated":"2014-11-23 13:29:18.000000000","message":"I\u0027m thinking just the opposite. :) That maybe this patch shouldn\u0027t address the notification logic at all, and instead just have an empty add_observer() method?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":569,"context_line":"        for observer in self.observers:"},{"line_number":570,"context_line":"            getattr(observer, notification)(*args)"},{"line_number":571,"context_line":""},{"line_number":572,"context_line":"    def _fip_ns_subscribe(self, router_id):"},{"line_number":573,"context_line":"        is_first \u003d (len(self.fip_ns_subscribers) \u003d\u003d 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_20a31e01","line":570,"in_reply_to":"5a890539_cb4363ad","updated":"2014-11-23 10:14:23.000000000","message":"I think that this patch\u0027s scope should be expanded a smudge... Mostly making sure that all notifications are fired, even if one fails, then raising an exception at the end so that the operation itself fails.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c733df361a2e8b3dfdf06455fe8f7a39f442d43b","unresolved":false,"context_lines":[{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def notify_observers(self, notification, *args):"},{"line_number":569,"context_line":"        for observer in self.observers:"},{"line_number":570,"context_line":"            getattr(observer, notification)(*args)"},{"line_number":571,"context_line":""},{"line_number":572,"context_line":"    def _fip_ns_subscribe(self, router_id):"},{"line_number":573,"context_line":"        is_first \u003d (len(self.fip_ns_subscribers) \u003d\u003d 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_cb4363ad","line":570,"in_reply_to":"5a890539_d54b0785","updated":"2014-11-22 15:38:11.000000000","message":"Yeah, I didn\u0027t put much thought/effort into the handling of notifications. In this patch I was mostly trying to setup the class hierarchy for the observer as phase 1, but wanted to show one example of a notification, just to give a feel for how it could be used.\n\nThe intent is that a follow up patch would deal with the actual notification processing. Is it OK to defer those details to that follow on effort (phase 2)?\n\nI see other potential concerns/questions here as well. Like, do we need to impose an ordering on the notifications to observers? Would there be more than one observer for the actions or just one? Is any rollback needed (I don\u0027t think there is currently)?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"90772da8ac449d87318923cf9c86d830206ddb0d","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_5578f7c3","line":760,"updated":"2014-11-22 04:34:24.000000000","message":"this (and like 786) is a bit messy, and could in theory be done with a decorator","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"68b2463ff322a7f6ea3ce0c6040c530a68eae541","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_74cdebb6","line":760,"in_reply_to":"5a890539_0038e2ed","updated":"2014-11-23 14:22:21.000000000","message":"I\u0027m going to pull the notification code from the change set. Will push another version in a bit and will work on doing concrete observer for other services in subsequent patches. Will leave notification for another refactoring as I think there is more thought needed there.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_80adb2eb","line":760,"in_reply_to":"5a890539_4b0f7388","updated":"2014-11-23 10:14:23.000000000","message":"Why not do it via a decorator in this patch?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c733df361a2e8b3dfdf06455fe8f7a39f442d43b","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_4b0f7388","line":760,"in_reply_to":"5a890539_5578f7c3","updated":"2014-11-22 15:38:11.000000000","message":"Yes that\u0027s a better way. Again, I was just trying to do a single example of using the notification. I\u0027d suspect that this would be tossed and redone in phase 2 of the refactoring.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_0038e2ed","line":760,"in_reply_to":"5a890539_80adb2eb","updated":"2014-11-23 13:29:18.000000000","message":"I could. I\u0027m just wondering if there should even be notification logic in this commit, or if I should remove it all and we address in subsequent refactorings.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"1dd097759b1289dcbd02ca73ec3fc193ca748fad","unresolved":false,"context_lines":[{"line_number":757,"context_line":"                    raise Exception(msg)"},{"line_number":758,"context_line":""},{"line_number":759,"context_line":"    def _router_added(self, router_id, router):"},{"line_number":760,"context_line":"        self.notify_observers(\u0027before_router_add\u0027, router_id, router)"},{"line_number":761,"context_line":"        ns_name \u003d (self.get_ns_name(router_id)"},{"line_number":762,"context_line":"                   if self.conf.use_namespaces else None)"},{"line_number":763,"context_line":"        ri \u003d RouterInfo(router_id\u003drouter_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_54070f18","line":760,"in_reply_to":"5a890539_80adb2eb","updated":"2014-11-23 13:20:29.000000000","message":"So, here\u0027s one reason why not to do it with a decorator: before_router_add needs to accept a RouterInfo object, not a router dict. self.notify_observers(\u0027before_router_add\u0027, router_id, router) needs to happen on line 768.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"90772da8ac449d87318923cf9c86d830206ddb0d","unresolved":false,"context_lines":[{"line_number":783,"context_line":"                self._add_keepalived_notifiers(ri)"},{"line_number":784,"context_line":"            else:"},{"line_number":785,"context_line":"                self._spawn_metadata_proxy(ri.router_id, ri.ns_name)"},{"line_number":786,"context_line":"        self.notify_observers(\u0027after_router_add\u0027, router_id, router)"},{"line_number":787,"context_line":""},{"line_number":788,"context_line":"    def _router_removed(self, router_id):"},{"line_number":789,"context_line":"        ri \u003d self.router_info.get(router_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_755c134c","line":786,"updated":"2014-11-22 04:34:24.000000000","message":"what if something fails here?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c733df361a2e8b3dfdf06455fe8f7a39f442d43b","unresolved":false,"context_lines":[{"line_number":783,"context_line":"                self._add_keepalived_notifiers(ri)"},{"line_number":784,"context_line":"            else:"},{"line_number":785,"context_line":"                self._spawn_metadata_proxy(ri.router_id, ri.ns_name)"},{"line_number":786,"context_line":"        self.notify_observers(\u0027after_router_add\u0027, router_id, router)"},{"line_number":787,"context_line":""},{"line_number":788,"context_line":"    def _router_removed(self, router_id):"},{"line_number":789,"context_line":"        ri \u003d self.router_info.get(router_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_cb1a83c5","line":786,"in_reply_to":"5a890539_755c134c","updated":"2014-11-22 15:38:11.000000000","message":"A good question. I don\u0027t know if in all cases things are rolled back (don\u0027t think so), stop on failure, or are best effort. Will likely need a lot of collaboration with the services during phase 2 of the refactoring to resolve this.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"dfe4f4a5e94cde7da3723255a7f6a8a078c80f49","unresolved":false,"context_lines":[{"line_number":561,"context_line":"        self.use_ipv6 \u003d ipv6_utils.is_enabled()"},{"line_number":562,"context_line":""},{"line_number":563,"context_line":"    def add_observer(self, observer, service_type):"},{"line_number":564,"context_line":"        \"\"\"Services that will get notified for events.\"\"\""},{"line_number":565,"context_line":"        # TODO(pcm): Implement in next refactoring phase"},{"line_number":566,"context_line":""},{"line_number":567,"context_line":"    def _fip_ns_subscribe(self, router_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_d2da184a","line":564,"updated":"2014-11-24 17:03:39.000000000","message":"I\u0027m working on FW and LB and due to FW being a base class of L3NATAgent, this is going to need to (temporarily) move.","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":769,"context_line":"            if ri.is_ha:"},{"line_number":770,"context_line":"                self._add_keepalived_notifiers(ri)"},{"line_number":771,"context_line":"            else:"},{"line_number":772,"context_line":"                self._spawn_metadata_proxy(ri.router_id, ri.ns_name)"},{"line_number":773,"context_line":""},{"line_number":774,"context_line":"    def _router_removed(self, router_id):"},{"line_number":775,"context_line":"        ri \u003d self.router_info.get(router_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_1a3c4030","line":772,"updated":"2014-12-06 00:49:50.000000000","message":"For VPN agent, the after_router_add notify should be here. Where it is currently loaded would change the calling sequence, such that the VPN actions would occur AFTER the L3 agent process_router() actions. I\u0027m not sure, whether or not that is OK, so safest to place it here. See my comment on the existing after_router_add notification.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":1808,"context_line":"        self._router_added(router[\u0027id\u0027], router)"},{"line_number":1809,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"},{"line_number":1810,"context_line":"        ri.router \u003d router"},{"line_number":1811,"context_line":"        # TODO(pcm): Can this be located here instead of in _router_added?"},{"line_number":1812,"context_line":"        # self.event_observers.notify("},{"line_number":1813,"context_line":"        #     adv_svc.AdvancedService.before_router_add, ri)"},{"line_number":1814,"context_line":"        self.process_router(ri)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_1a2280ba","line":1811,"updated":"2014-12-06 00:49:50.000000000","message":"See below.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":1813,"context_line":"        #     adv_svc.AdvancedService.before_router_add, ri)"},{"line_number":1814,"context_line":"        self.process_router(ri)"},{"line_number":1815,"context_line":"        self.event_observers.notify("},{"line_number":1816,"context_line":"            adv_svc.AdvancedService.after_router_add, ri)"},{"line_number":1817,"context_line":""},{"line_number":1818,"context_line":"    def _process_updated_router(self, router):"},{"line_number":1819,"context_line":"        ri \u003d self.router_info[router[\u0027id\u0027]]"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_1a6a801b","line":1816,"updated":"2014-12-06 00:49:50.000000000","message":"For VPN this notification is needed for the processing to be done as part of the VPNs _process_router_if_compatible. I propose that we rename this after_process_router_for_add (and the ones on L1822, 1825 could be {before,after}_process_router_for_update). A corresponding before_process_router_for_add() can be placed on L1813.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"}],"neutron/services/advanced_service.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2012 OpenStack Foundation."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_6097e690","line":1,"updated":"2014-11-23 10:14:23.000000000","message":"2014","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2012 OpenStack Foundation."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_20395ef2","line":1,"in_reply_to":"5a890539_6097e690","updated":"2014-11-23 13:29:18.000000000","message":"Oops. Copy and paste.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"90772da8ac449d87318923cf9c86d830206ddb0d","unresolved":false,"context_lines":[{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":26,"context_line":"class AdvancedService(object):"},{"line_number":27,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_7575f3c9","line":25,"updated":"2014-11-22 04:34:24.000000000","message":"IMO, I would advise against the use of a metaclass, as they are an overkill for this case.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c733df361a2e8b3dfdf06455fe8f7a39f442d43b","unresolved":false,"context_lines":[{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":26,"context_line":"class AdvancedService(object):"},{"line_number":27,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_eb61ff39","line":25,"in_reply_to":"5a890539_7575f3c9","updated":"2014-11-22 15:38:11.000000000","message":"Agree. Will pull that as it won\u0027t be needed. We don\u0027t want the child classes to have to implement every method - only the ones they are interested in. Will change in next patch, when I add more services.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":26,"context_line":"class AdvancedService(object):"},{"line_number":27,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_600d263e","line":25,"in_reply_to":"5a890539_a0c54e5d","updated":"2014-11-23 13:29:18.000000000","message":"Not adversed to moving, but I was thinking about trying to keep everything service related under neutron/services/ (and it eliminates a patch dependency). Especially, if later, advanced services is moved to a new repo. Thoughts?","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cdc503ea8b264834d0d2c35da30d49ebf9a04d09","unresolved":false,"context_lines":[{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":26,"context_line":"class AdvancedService(object):"},{"line_number":27,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_a0c54e5d","line":25,"in_reply_to":"5a890539_eb61ff39","updated":"2014-11-23 10:49:01.000000000","message":"I think that you should do a \"soft dependency\" on:\n\nhttps://review.openstack.org/#/c/130300/\n\nThen the advanced services should actually be placed at:\n\nneutron/agent/l3/services/*","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"f561c7dc051fca0cc9343036ff84ad34476b1bcc","unresolved":false,"context_lines":[{"line_number":19,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class AdvancedService(object):"},{"line_number":23,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    def __init__(self, l3_agent, service_type\u003dconstants.DUMMY):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_78a783a7","line":22,"updated":"2014-11-24 18:15:19.000000000","message":"Currently there is no point to this interface.  It calls an empty observer registration and VPNAgent takes care of calling load_device_drivers on its own.","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"5ae46a0032e51f87dcd319fe38e4759d62afb0f8","unresolved":false,"context_lines":[{"line_number":19,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class AdvancedService(object):"},{"line_number":23,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    def __init__(self, l3_agent, service_type\u003dconstants.DUMMY):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_3887bbb6","line":22,"in_reply_to":"5a890539_78a783a7","updated":"2014-11-24 18:33:02.000000000","message":"It\u0027s the place holder for the notification methods for the observer class and it\u0027s the base for the child classes that will implement the handlers and do per service specific loading. Are you suggesting removing it?","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"63b051d8374a9e79b18a6b132b97a3d04932822c","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class AdvancedService(object):"},{"line_number":23,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    def __init__(self, l3_agent, service_type\u003dconstants.DUMMY):"},{"line_number":26,"context_line":"        l3_agent.add_observer(self, service_type)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_80467f79","line":24,"updated":"2014-11-24 14:17:22.000000000","message":"service_type isn\u0027t used in this patch. What did you had in mind? Is it to prevent the same driver registering more than once? If so let\u0027s handle that (If at all) in a patch that does registration properly.","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"dfe4f4a5e94cde7da3723255a7f6a8a078c80f49","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class AdvancedService(object):"},{"line_number":23,"context_line":"    \"\"\"Observer base class for Advanced Services.\"\"\""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    def __init__(self, l3_agent, service_type\u003dconstants.DUMMY):"},{"line_number":26,"context_line":"        l3_agent.add_observer(self, service_type)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_d223f855","line":24,"in_reply_to":"5a890539_80467f79","updated":"2014-11-24 17:03:39.000000000","message":"Yeah, left over from the previous discussion, where I was changing the observer list to a dict to prevent duplicate entries. Will pull.","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"        return cls._instance"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def before_router_add(self, ri):"},{"line_number":76,"context_line":"        pass"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def after_router_add(self, ri):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_5aa7a850","line":75,"updated":"2014-12-06 00:49:50.000000000","message":"For each of these event handlers, there should be a docstring explaining when they are called (where they are hooked into). For example:\n\n    \"\"\"Actions taken before router_info created.\"\"\"\n    \"\"\"Actions taken after router_info created.\"\"\"\n    \"\"\"Actions before processing for an updated router.\"\"\"\n    \"\"\"Actions after processing for an updated router.\"\"\"\n    \"\"\"Actions before removing router.\"\"\"\n    \"\"\"Actions after processing and removing router.\"\"\"\n\nFor new proposed methods:\n    def before_process_router_for_add(self, ri):\n        \"\"\"Actions taken before processing router for add.\"\"\"\n        pass\n\n    def after_process_router_for_add(self, ri):\n        \"\"\"Actions taken after processing router for add.\"\"\"\n        pass\n\nWe could elaborate more about the context of the action, if desired.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":78,"context_line":"    def after_router_add(self, ri):"},{"line_number":79,"context_line":"        pass"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def before_router_update(self, ri):"},{"line_number":82,"context_line":"        pass"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def after_router_update(self, ri):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_ba4c0c9f","line":81,"updated":"2014-12-06 00:49:50.000000000","message":"Propose: before_processing_router_for_update","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":81,"context_line":"    def before_router_update(self, ri):"},{"line_number":82,"context_line":"        pass"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def after_router_update(self, ri):"},{"line_number":85,"context_line":"        pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    def before_router_remove(self, ri):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_fa46947a","line":84,"updated":"2014-12-06 00:49:50.000000000","message":"Propose: after_processing_router_for_update","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"97ed11441a8a4a26dda07ef62d7b616558a2220f","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        specific service type."},{"line_number":55,"context_line":"        \"\"\""},{"line_number":56,"context_line":"        if not cls._instance:"},{"line_number":57,"context_line":"            with lockutils.lock(\u0027instance\u0027):"},{"line_number":58,"context_line":"                if not cls._instance:"},{"line_number":59,"context_line":"                    cls._instance \u003d cls(l3_agent)"},{"line_number":60,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_704b7d87","line":57,"updated":"2014-12-09 17:58:54.000000000","message":"Since this is just a semaphore, not an external file-based lock, is the extra if() above really worth it?  It should be very quick assuming this method isn\u0027t called that often.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"ed5bf1424d27f5735e517805766ae77f6b3a7146","unresolved":false,"context_lines":[{"line_number":54,"context_line":"        specific service type."},{"line_number":55,"context_line":"        \"\"\""},{"line_number":56,"context_line":"        if not cls._instance:"},{"line_number":57,"context_line":"            with lockutils.lock(\u0027instance\u0027):"},{"line_number":58,"context_line":"                if not cls._instance:"},{"line_number":59,"context_line":"                    cls._instance \u003d cls(l3_agent)"},{"line_number":60,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_392f21f8","line":57,"in_reply_to":"3a961159_704b7d87","updated":"2014-12-09 18:36:24.000000000","message":"Probably not, given the amount of calls to this, but will leave this in, as a common pattern for double checked locking.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"2ea753dded8376a04e05055f5239275787d1badb","unresolved":false,"context_lines":[{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        return cls._instance"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def before_router_added(self, ri):"},{"line_number":64,"context_line":"        \"\"\"Actions taken before router_info created.\"\"\""},{"line_number":65,"context_line":"        pass"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"3a961159_b00770c9","line":63,"updated":"2014-12-10 23:16:52.000000000","message":"iirc, method names inherit from current l3 agents method names but\ni would prefer we use clearer method names as the docstring do not help to understand the difference between router_added and process_router_for_add steps without looking at code.","commit_id":"fee1c41cd7b2e26c9581f6b6602c6c9efaed6073"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"d39a9eb437dc6d05d64d4128c5ae34c9edcab0c7","unresolved":false,"context_lines":[{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        return cls._instance"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def before_router_added(self, ri):"},{"line_number":64,"context_line":"        \"\"\"Actions taken before router_info created.\"\"\""},{"line_number":65,"context_line":"        pass"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"3a961159_2b04f548","line":63,"in_reply_to":"3a961159_b00770c9","updated":"2014-12-11 00:03:48.000000000","message":"These are not intended to map directly to the L3 agent method names (_router_added, _router_removed, _process_router_if_compatible). The \"intent\" is to have methods that map to the event notifications that the L3 agent will generate.\n\nSo, during creation of router, there will be several points where services need to take action. They\u0027ll register as observers, and then will implement the corresponding handler.\n\nIn addition, we\u0027re still refactoring (incrementally), and the goal here is to reduce the number of notifications that will happen. Hopefully, there will just be a pair for add/remove/update.\n\nHowever, to fit with what is there right now, we have handlers that are mapping to various points (e.g. some around the _router_added, and some around the _process_added_router) In fact, it looks like some of those methods will be removed/reduced.\n\nSo, there will be several iterations here on the methods.","commit_id":"fee1c41cd7b2e26c9581f6b6602c6c9efaed6073"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"4f1a1526f7bd2c864b27bd827e23e47d335e4a33","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    that a service could \"observe\". A child class for a service type will"},{"line_number":33,"context_line":"    implement handlers, for events of interest."},{"line_number":34,"context_line":"    \"\"\""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    _instance \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __init__(self, l3_agent):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_89b84712","line":35,"updated":"2014-12-12 20:04:59.000000000","message":"This should be at the module level, not the class level. It should not make a functional difference, but it\u0027s \"The Python Way (TM)\".","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"1167244e04fb6e8d7f9a238126adf9e4333b495b","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    that a service could \"observe\". A child class for a service type will"},{"line_number":33,"context_line":"    implement handlers, for events of interest."},{"line_number":34,"context_line":"    \"\"\""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    _instance \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __init__(self, l3_agent):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_8f29279d","line":35,"in_reply_to":"3a961159_2feaf3da","updated":"2014-12-12 21:26:02.000000000","message":"I thought Assaf meant the _instance declaration below.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    that a service could \"observe\". A child class for a service type will"},{"line_number":33,"context_line":"    implement handlers, for events of interest."},{"line_number":34,"context_line":"    \"\"\""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    _instance \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __init__(self, l3_agent):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_2feaf3da","line":35,"in_reply_to":"3a961159_89b84712","updated":"2014-12-12 21:13:46.000000000","message":"I beg to differ. This is a docstring for the class and not the module. If, you have a reference that indicates that it should be at the module let, please advise and I can update. I was following (loosely) PEP257.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"34a6980641da0e8f116ab0468bb68ab5bcffdf7c","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    that a service could \"observe\". A child class for a service type will"},{"line_number":33,"context_line":"    implement handlers, for events of interest."},{"line_number":34,"context_line":"    \"\"\""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    _instance \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __init__(self, l3_agent):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_cc697dfa","line":35,"in_reply_to":"3a961159_89b84712","updated":"2014-12-12 20:54:23.000000000","message":"Really?  Ok.  I can go either way.  I am interested in seeing a reference that says so.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c3502b58f6dfcfd0479261354536ca4155b9f3bd","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    that a service could \"observe\". A child class for a service type will"},{"line_number":33,"context_line":"    implement handlers, for events of interest."},{"line_number":34,"context_line":"    \"\"\""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    _instance \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __init__(self, l3_agent):"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_faa18746","line":35,"in_reply_to":"3a961159_8f29279d","updated":"2014-12-12 22:02:19.000000000","message":"Ah. I thought the comment was for the line above and not below. I\u0027ve seen it used inside the class. Will leave as-is. Can be changed in later refactorings, if there is a definitive direction.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a948076459ccbd2abdaaa1535a9c84d2beb14058","unresolved":false,"context_lines":[{"line_number":44,"context_line":"        self.l3_agent \u003d l3_agent"},{"line_number":45,"context_line":"        # NOTE: Copying L3 agent attributes, so that they are accessible"},{"line_number":46,"context_line":"        # from device drivers, which are now provided a service instance."},{"line_number":47,"context_line":"        self.conf \u003d l3_agent.conf"},{"line_number":48,"context_line":"        self.root_helper \u003d l3_agent.root_helper"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_e5be51ad","line":47,"updated":"2014-12-12 18:56:19.000000000","message":"NOTE:  We\u0027re going to want to work on this in future patches.  However, I\u0027m good with these two lines here because they are needed for now.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":44,"context_line":"        self.l3_agent \u003d l3_agent"},{"line_number":45,"context_line":"        # NOTE: Copying L3 agent attributes, so that they are accessible"},{"line_number":46,"context_line":"        # from device drivers, which are now provided a service instance."},{"line_number":47,"context_line":"        self.conf \u003d l3_agent.conf"},{"line_number":48,"context_line":"        self.root_helper \u003d l3_agent.root_helper"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_0fb7f7f5","line":47,"in_reply_to":"3a961159_e5be51ad","updated":"2014-12-12 21:13:46.000000000","message":"Will add TODO for cleanup.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"afde848c9aabda34961b2410fa0cc10bfb8b10ab","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    # some services may want to take different actions."},{"line_number":71,"context_line":"    def before_router_added(self, ri):"},{"line_number":72,"context_line":"        \"\"\"Actions taken before router_info created.\"\"\""},{"line_number":73,"context_line":"        pass"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def after_router_added(self, ri):"},{"line_number":76,"context_line":"        \"\"\"Actions taken after router_info created.\"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_0f3e17da","line":73,"updated":"2014-12-12 21:26:48.000000000","message":"pass is not required","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"c3502b58f6dfcfd0479261354536ca4155b9f3bd","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    # some services may want to take different actions."},{"line_number":71,"context_line":"    def before_router_added(self, ri):"},{"line_number":72,"context_line":"        \"\"\"Actions taken before router_info created.\"\"\""},{"line_number":73,"context_line":"        pass"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def after_router_added(self, ri):"},{"line_number":76,"context_line":"        \"\"\"Actions taken after router_info created.\"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_f5435810","line":73,"in_reply_to":"3a961159_0f3e17da","updated":"2014-12-12 22:02:19.000000000","message":"With the docstring it\u0027s not required, but I\u0027d like to keep it, as I like the explicit indication (just a personal preference on my part). If you feel strongly on it, I can change it. Just let me know soon, if you can, as I want to push up the change.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"4f1a1526f7bd2c864b27bd827e23e47d335e4a33","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    def after_router_updated(self, ri):"},{"line_number":84,"context_line":"        \"\"\"Actions add processing for an updated router.\"\"\""},{"line_number":85,"context_line":"        pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    def before_router_remove(self, ri):"},{"line_number":88,"context_line":"        \"\"\"Actions before removing router.\"\"\""},{"line_number":89,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_09a8d7d9","line":86,"updated":"2014-12-12 20:04:59.000000000","message":"remove -\u003e removed (For consistency).","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"34a6980641da0e8f116ab0468bb68ab5bcffdf7c","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    def after_router_updated(self, ri):"},{"line_number":84,"context_line":"        \"\"\"Actions add processing for an updated router.\"\"\""},{"line_number":85,"context_line":"        pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    def before_router_remove(self, ri):"},{"line_number":88,"context_line":"        \"\"\"Actions before removing router.\"\"\""},{"line_number":89,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_0c70e5da","line":86,"in_reply_to":"3a961159_09a8d7d9","updated":"2014-12-12 20:54:23.000000000","message":"+1  Sorry I missed that in my patch.","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"a508bf7e1d613237fcda9e189b97376fad4d4b84","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    def after_router_updated(self, ri):"},{"line_number":84,"context_line":"        \"\"\"Actions add processing for an updated router.\"\"\""},{"line_number":85,"context_line":"        pass"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    def before_router_remove(self, ri):"},{"line_number":88,"context_line":"        \"\"\"Actions before removing router.\"\"\""},{"line_number":89,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a961159_8fc20751","line":86,"in_reply_to":"3a961159_09a8d7d9","updated":"2014-12-12 21:13:46.000000000","message":"Done","commit_id":"2ab09782cce237e0e4eb6d51086b0b7211481741"}],"neutron/services/firewall/agents/firewall_service.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"9836c96a25ba3796dc39fa54183b0fafa44bb64c","unresolved":false,"context_lines":[{"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):"},{"line_number":29,"context_line":"        super(FirewallService, self).__init__(l3_agent)"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def load_device_drivers(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_be1f61b6","line":28,"updated":"2014-12-01 20:55:00.000000000","message":"this method doesn\u0027t do anything","commit_id":"6b2b56d1126f194073212e6240278b9c1043cca5"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"77af09e6891205f09d6122f759fc825ff607c4c7","unresolved":false,"context_lines":[{"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):"},{"line_number":29,"context_line":"        super(FirewallService, self).__init__(l3_agent)"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def load_device_drivers(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a890539_9d4aea59","line":28,"in_reply_to":"5a890539_be1f61b6","updated":"2014-12-06 00:55:08.000000000","message":"Done","commit_id":"6b2b56d1126f194073212e6240278b9c1043cca5"}],"neutron/services/firewall/agents/l3reference/firewall_l3_agent.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d94c87c2ec386f6b230e11d9860fad1249cee30c","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        if self.fwaas_enabled:"},{"line_number":73,"context_line":"            # NOTE: Temp location for creating service and loading driver"},{"line_number":74,"context_line":"            self.fw_service \u003d firewall_service.FirewallService(self)"},{"line_number":75,"context_line":"            self.fwaas_driver \u003d self.fw_service.load_device_drivers()"},{"line_number":76,"context_line":"        self.services_sync \u003d False"},{"line_number":77,"context_line":"        self.root_helper \u003d config.get_root_helper(conf)"},{"line_number":78,"context_line":"        # setup RPC to msg fwaas plugin"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a890539_a680338c","line":75,"updated":"2014-12-02 00:44:56.000000000","message":"Should each service just load its device drivers when it is initialized?  Is there a reason to separate initialization of the service and loading the device drivers?\n\nOne reason to combine them might be to simplify the interface.","commit_id":"98b6725bdd9a1491d99da0f26c45c2e633a62c36"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"dfe112557dca71695da72fa882e1cf891740c24a","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        if self.fwaas_enabled:"},{"line_number":73,"context_line":"            # NOTE: Temp location for creating service and loading driver"},{"line_number":74,"context_line":"            self.fw_service \u003d firewall_service.FirewallService(self)"},{"line_number":75,"context_line":"            self.fwaas_driver \u003d self.fw_service.load_device_drivers()"},{"line_number":76,"context_line":"        self.services_sync \u003d False"},{"line_number":77,"context_line":"        self.root_helper \u003d config.get_root_helper(conf)"},{"line_number":78,"context_line":"        # setup RPC to msg fwaas plugin"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a890539_b45118db","line":75,"in_reply_to":"5a890539_a680338c","updated":"2014-12-02 21:12:52.000000000","message":"I was separating the two, because, in the interim steps here, agents like FW and VPN want the list of the loaded drivers returned, so this gives the ability to do that. It also makes testing easier for the load_device_driver method.\n\nGranted, we could load at creation and provide a get method, but it results in the same two calls.","commit_id":"98b6725bdd9a1491d99da0f26c45c2e633a62c36"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"fcca1a8d2817b9eb942cee3cba334430f302b330","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        if self.fwaas_enabled:"},{"line_number":73,"context_line":"            # NOTE: Temp location for creating service and loading driver"},{"line_number":74,"context_line":"            self.fw_service \u003d firewall_service.FirewallService(self)"},{"line_number":75,"context_line":"            self.fwaas_driver \u003d self.fw_service.load_device_drivers()"},{"line_number":76,"context_line":"        self.services_sync \u003d False"},{"line_number":77,"context_line":"        self.root_helper \u003d config.get_root_helper(conf)"},{"line_number":78,"context_line":"        # setup RPC to msg fwaas plugin"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a890539_e430e5ea","line":75,"in_reply_to":"5a890539_b45118db","updated":"2014-12-05 00:07:43.000000000","message":"Thinking about it more, I\u0027m fine with this call.","commit_id":"98b6725bdd9a1491d99da0f26c45c2e633a62c36"}],"neutron/services/vpn/agent.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"f561c7dc051fca0cc9343036ff84ad34476b1bcc","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        super(VPNAgent, self).__init__(host\u003dhost, conf\u003dconf)"},{"line_number":35,"context_line":"        # TODO(pcm) Remove these, once L3 agent calls service factory"},{"line_number":36,"context_line":"        self.service \u003d vpn_service.VPNService(self)"},{"line_number":37,"context_line":"        self.devices \u003d self.service.load_device_drivers(self, host)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    def get_namespace(self, router_id):"},{"line_number":40,"context_line":"        \"\"\"Get namespace of router."}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_d8add785","line":37,"updated":"2014-11-24 18:15:19.000000000","message":"What is the point of defining load_device_drivers in the abstract interface if each service is going to call it for itself?","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"5ae46a0032e51f87dcd319fe38e4759d62afb0f8","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        super(VPNAgent, self).__init__(host\u003dhost, conf\u003dconf)"},{"line_number":35,"context_line":"        # TODO(pcm) Remove these, once L3 agent calls service factory"},{"line_number":36,"context_line":"        self.service \u003d vpn_service.VPNService(self)"},{"line_number":37,"context_line":"        self.devices \u003d self.service.load_device_drivers(self, host)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    def get_namespace(self, router_id):"},{"line_number":40,"context_line":"        \"\"\"Get namespace of router."}],"source_content_type":"text/x-python","patch_set":2,"id":"5a890539_63b3544d","line":37,"in_reply_to":"5a890539_d8add785","updated":"2014-11-24 18:33:02.000000000","message":"Hmm. Initially, I had the VPNService() creation loading the device drivers. However, to fit in with the existing agents, I needed to return the drivers loaded, so I split it out. It also made it easier to test the load method.\n\nIt may not make sense to have it in the base class, which means base class is empty at this time, other than calling the add_observer().","commit_id":"c1e1a2bf500bc0bfe2e1a3183ba73936d765521d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"38fc51380ac8ca89ecabb0aaef59fbff099ee277","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        self.event_observers.add(self.service)"},{"line_number":39,"context_line":"        self.devices \u003d self.service.load_device_drivers(host)"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def get_namespace(self, router_id):"},{"line_number":42,"context_line":"        \"\"\"Get namespace of router."},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        :router_id: router_id"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_76a9b6de","line":41,"updated":"2014-12-09 18:48:30.000000000","message":"I guess the next step would be to move all of these methods to the service class.  I guess I would be okay if that is done in this patch or a follow-on patch.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"2da3886a25b430a032639b63dd1fbce8d3f9f352","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        self.event_observers.add(self.service)"},{"line_number":39,"context_line":"        self.devices \u003d self.service.load_device_drivers(host)"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def get_namespace(self, router_id):"},{"line_number":42,"context_line":"        \"\"\"Get namespace of router."},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        :router_id: router_id"}],"source_content_type":"text/x-python","patch_set":10,"id":"3a961159_7ff7c9f5","line":41,"in_reply_to":"3a961159_76a9b6de","updated":"2014-12-09 19:18:49.000000000","message":"Correct, that would be the next step. I\u0027ll start doing that under this commit, unless there is any desire to use another commit.","commit_id":"a8e184914c2b447e3cd4641304599e2a4864f652"}],"neutron/services/vpn/vpn_service.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2012 OpenStack Foundation."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_8094d28b","line":1,"updated":"2014-11-23 10:14:23.000000000","message":"2014","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2012 OpenStack Foundation."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_c0009a04","line":1,"in_reply_to":"5a890539_8094d28b","updated":"2014-11-23 13:29:18.000000000","message":"Done","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    distributed under the License is distributed on an \"AS IS\" BASIS, WITHOUT"},{"line_number":12,"context_line":"#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the"},{"line_number":13,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":14,"context_line":"#    under the License."},{"line_number":15,"context_line":"from oslo.config import cfg"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron.extensions import vpnaas"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_a08d4e7e","line":14,"updated":"2014-11-23 10:14:23.000000000","message":"space after copyright crap.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    distributed under the License is distributed on an \"AS IS\" BASIS, WITHOUT"},{"line_number":12,"context_line":"#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the"},{"line_number":13,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":14,"context_line":"#    under the License."},{"line_number":15,"context_line":"from oslo.config import cfg"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron.extensions import vpnaas"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_3406031e","line":14,"in_reply_to":"5a890539_a08d4e7e","updated":"2014-11-23 13:29:18.000000000","message":"Done","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3b0af05b39f851d21656a6b3e649cedcf10ea00f","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class VPNService(advanced_service.AdvancedService):"},{"line_number":26,"context_line":"    \"\"\"VPN Service observer.\"\"\""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_a05f0e0e","line":24,"updated":"2014-11-23 10:14:23.000000000","message":"Let\u0027s assume that https://review.openstack.org/#/c/130300/ is merged, why not put this file under neutron/agent/l3/services/vpn.py?\n\nIt\u0027s more intuitive for me to search for an agent-side implementation of a service in an agent subdirectory.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class VPNService(advanced_service.AdvancedService):"},{"line_number":26,"context_line":"    \"\"\"VPN Service observer.\"\"\""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_74232ba7","line":24,"in_reply_to":"5a890539_a05f0e0e","updated":"2014-11-23 13:29:18.000000000","message":"I was thinking about advanced services spin-out, and trying to keep all advanced services under the neutron/services subtree (where it exists today).","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"9836c96a25ba3796dc39fa54183b0fafa44bb64c","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):"},{"line_number":30,"context_line":"        super(VPNService, self).__init__(l3_agent)"},{"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":5,"id":"5a890539_de1a6dc5","line":29,"updated":"2014-12-01 20:55:00.000000000","message":"noop, remove","commit_id":"6b2b56d1126f194073212e6240278b9c1043cca5"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"77af09e6891205f09d6122f759fc825ff607c4c7","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):"},{"line_number":30,"context_line":"        super(VPNService, self).__init__(l3_agent)"},{"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":5,"id":"5a890539_dd547200","line":29,"in_reply_to":"5a890539_de1a6dc5","updated":"2014-12-06 00:55:08.000000000","message":"Done","commit_id":"6b2b56d1126f194073212e6240278b9c1043cca5"}],"neutron/tests/functional/agent/test_l3_agent.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"3e84fdd95a2d1b852d08f043d2c02ee824a4d36d","unresolved":false,"context_lines":[{"line_number":207,"context_line":"            self.assertEqual("},{"line_number":208,"context_line":"                [((adv_svc.AdvancedService.before_router_add, router),),"},{"line_number":209,"context_line":"                 ((adv_svc.AdvancedService.after_router_add, router),),"},{"line_number":210,"context_line":"                 ((adv_svc.AdvancedService.before_router_update, router),),"},{"line_number":211,"context_line":"                 ((adv_svc.AdvancedService.after_router_update, router),),"},{"line_number":212,"context_line":"                 ((adv_svc.AdvancedService.before_router_remove, router),),"},{"line_number":213,"context_line":"                 ((adv_svc.AdvancedService.after_router_remove, router),)],"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a890539_5a032810","line":210,"updated":"2014-12-06 00:49:50.000000000","message":"These two lines would change with proposal. And new ones added.","commit_id":"40a5522079310de7534050401c64bf6cb0c6044e"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"8ef7f1697672ff369a3924dab68c778edcc0db03","unresolved":false,"context_lines":[{"line_number":188,"context_line":""},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"class L3AgentTestCase(L3AgentTestFramework):"},{"line_number":191,"context_line":"    def test_driver_notifications_legacy_router(self):"},{"line_number":192,"context_line":"        self._test_driver_notifications(enable_ha\u003dFalse)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def test_driver_notifications_ha_router(self):"}],"source_content_type":"text/x-python","patch_set":8,"id":"3a961159_fc720ae4","line":191,"updated":"2014-12-08 22:52:27.000000000","message":"Nit: should we rename these test_notifying_agents_using_legacy_router(), test_notifying_agents_using_ha_router(), _test_notifying_agents() or with observers vs agents?","commit_id":"783340335380dcc490b8f031b477725ce85ed11b"}],"neutron/tests/unit/services/vpn/test_vpn_service.py":[{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"1758896c0420b11d74dcde7f24449da79ad711f7","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright 2012 OpenStack Foundation."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a890539_943b37d0","line":1,"updated":"2014-11-23 13:29:18.000000000","message":"Fixed.","commit_id":"0793bd87a8191fe82d99964a093630d95f542950"}]}