)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"7d6c0a4fde821a7777232633756d4d688725dd7a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Pass rule object to rule_delete method in QoS L2 drivers"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In some cases it\u0027s necessary to pass rule object to"},{"line_number":10,"context_line":"_handle_rule_delete method."},{"line_number":11,"context_line":"Rule can be used then to call drivers method based on values"},{"line_number":12,"context_line":"of rule attributes."},{"line_number":13,"context_line":"For example bandwidth limit rule have direction attribute and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"ff0f0b1f_c0442b77","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":27},"updated":"2017-05-26 17:28:56.000000000","message":"This commit message should be updated with what we are actually trying to do. We are trying to extennd the abstract interface for QoS Agent Drivers to enable the addition of deletion methods for ingress rules, without having to pass a rule object to it","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"af542fe200d035c02dabf022d0c55fcec4aeceb5","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Pass rule object to rule_delete method in QoS L2 drivers"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In some cases it\u0027s necessary to pass rule object to"},{"line_number":10,"context_line":"_handle_rule_delete method."},{"line_number":11,"context_line":"Rule can be used then to call drivers method based on values"},{"line_number":12,"context_line":"of rule attributes."},{"line_number":13,"context_line":"For example bandwidth limit rule have direction attribute and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"ff0f0b1f_87d9a5f5","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":27},"in_reply_to":"ff0f0b1f_c0442b77","updated":"2017-05-26 20:11:43.000000000","message":"changed","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8ffe46c485ab173442a129bedc31fd4c60c6c94e","unresolved":false,"context_lines":[{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accpets"},{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df140735_23cc0cbb","line":13,"range":{"start_line":13,"start_character":47,"end_line":13,"end_character":54},"updated":"2017-05-30 13:56:06.000000000","message":"Nit: accepts","commit_id":"9786aa795d9ce8d5854e3bc3bb140562d03e2f5d"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"d8ac07007d0ee4cc016e3d7c0f3270561af0f8bd","unresolved":false,"context_lines":[{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accpets"},{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df140735_065c7623","line":13,"range":{"start_line":13,"start_character":47,"end_line":13,"end_character":54},"in_reply_to":"df140735_23cc0cbb","updated":"2017-05-30 14:17:25.000000000","message":"Done","commit_id":"9786aa795d9ce8d5854e3bc3bb140562d03e2f5d"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"1e49ccd52cb299f3a5e718afea5a22fe2cd84390","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_a9e47bad","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"updated":"2017-05-30 15:01:33.000000000","message":"can you explain why only delete needs this special argument?\nisn\u0027t it cleaner to pass qos_policy as the other methods do?","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"a3d40d3444c7de928237e32125eec2ef081f6c5f","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_a0ad4eea","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_1d322560","updated":"2017-06-02 06:58:32.000000000","message":"yes, it should be a rule, not policy.\n\nright now create/update methods take a rule but delete doesn\u0027t, right?\nmy suggesting is to make delete take a rule as well.\nit\u0027s at least consistent.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"c231a947b84797849a9d82d5b6b37997e6ae68e6","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_6c9ccd69","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_5199a676","updated":"2017-06-02 09:15:03.000000000","message":"what\u0027s wrong with something like the following?\n\n  try:\n     handler(port, rule)\n  except TypeError:\n     handler(port)  # fallback to the old api","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"dd6938bb975395bda81468d1e294455e9436daf8","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_cc88d8c7","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_6c62f140","updated":"2017-05-30 18:39:53.000000000","message":"It\u0027s exactly how Rodolfo wrote. Problem with delete rule is that until now there was always strict constraint that there can be only one rule of \"rule_type\" (like bandwidth_limit_rule) in QoS policy so during delete_\u003crule_type\u003e it was easy to choose which rule should be deleted.\nNow when \"ingress\" direction was introduced to bandwidth_limit_rule QoS policy can have one such rule with ingress and one with egress direction.\nBecause of that there is need to tell somehow to handler method which rule should be deleted. Easiest way would be to pass rule object to delete_\u003crule_type\u003e handler method but that would break compatibility with third part drivers.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"ac0479b8950d6f6b19f7c636ddac3287ed6b79e1","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_9919a023","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_6c9ccd69","updated":"2017-06-02 11:11:09.000000000","message":"Ihar don\u0027t like such solution. I proposed it in one comment in https://review.openstack.org/#/c/457816/7/neutron/agent/l2/extensions/qos.py already :)","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"05cde9327b4f27be5f8b389aa200e9b57c6a8f6a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_ed4ca89f","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_960aeb23","updated":"2017-05-31 08:35:28.000000000","message":"\u003e  new create/update/delete methods for ingress rules\n\nFor all of them? I mean, create and update methods already have \"rule\" as a parameter (and therefore the direction of the rule).\n\nDo you think this will be better for all methods (create/update/delete)?\n\nThanks for your input.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"1c6c87adda61aabd7d405f302b6948407c250a09","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_1d322560","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_9d7cf51f","updated":"2017-06-02 06:36:56.000000000","message":"we should pass rule, not policy in such case and IMO it would be even more confusing for users.\nIMO it\u0027s better to tell that driver should handle differently rules with direction\u003dingress rather than pass rule object to some handler methods and not to others.\n\nI also agree with Ihar that it can be good idea to provide new handler methods for ingress rule types. I will do change it ASAP.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"8bf7ea043dc4beba378e930eee03f72e99cc3f93","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_5199a676","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_a0ad4eea","updated":"2017-06-02 08:57:12.000000000","message":"I agree that it would be best solution but whole this patch is because it would be breaking change for third part drivers. Discussion about it was in different patch: https://review.openstack.org/#/c/457816/7/neutron/agent/l2/extensions/qos.py\n\nThat\u0027s why Ihar and Miguel propose to do such \"workaround\" here.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"3a38a31f6fab1a10236f940445276250db050f31","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_6c62f140","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_a9e47bad","updated":"2017-05-30 15:26:11.000000000","message":"One year and a half ago (cc698b2ba578e5bc1475f6229bfebd1316c41ffb) the QoS agent was refactored. At this point, the concept of directionality wasn\u0027t considered:\n* \"update\" and \"create\" functions were implemented adding the rule as a input parameter. Using this rule the driver implementation can read the direction of this rule.\n* \"delete\" functions were implemented without this parameter.\n\nNow we need to add, at least, the direction parameter. To avoid any regression, including any external plugins, this implementation provides the directionality input to \"delete\" functions ONLY to those drivers which supports both directions.\n\nBecause now there is no driver supporting INGRESS direction, no modification is needed. Once we start adding the ingress direction for some rules and drivers, we need to modify the \"delete_\u003crule\u003e\" function to accept the direction as well.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"560fecead1c1a5b2cf948aab27ebba2e67bbe8b6","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_960aeb23","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_cc88d8c7","updated":"2017-05-30 21:53:15.000000000","message":"While technically you indeed don\u0027t need to pass it for other methods, it doesn\u0027t make for a great API that you optimize it this way.\n\nIf I were Slawek, I would introduce new create/update/delete methods for ingress rules. The benefit of it instead of a keyword is that then driver author will catch the mistake if he forgets to provide implementation, while with keyword, he may miss it (because existing methods may already have **kwargs catching all).","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"1eba3acc4a4f0ec03b79fcfb8e332bf17d2cb357","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Extend QoS L2 drivers interface to handle ingress rule types"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"df140735_9d7cf51f","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":24},"in_reply_to":"df140735_ed4ca89f","updated":"2017-06-02 06:31:58.000000000","message":"what\u0027s wrong with passing a policy for delete as we do for other methods? (rather than a boolean)\nfor compatibility you can have a fallback to the old api.","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"6460e7982619ccc756f372a558b1ab672033ed5d","unresolved":false,"context_lines":[{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"},{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"df140735_804861cc","line":13,"range":{"start_line":13,"start_character":41,"end_line":13,"end_character":46},"updated":"2017-05-30 20:20:45.000000000","message":"which","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"99cd6b405ed2ce14a4fd1b355cd39312f0125394","unresolved":false,"context_lines":[{"line_number":10,"context_line":"remove of ingress rules."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":13,"context_line":"L2 drivers should provide handler method witch accepts"},{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"df140735_56ed73ed","line":13,"range":{"start_line":13,"start_character":41,"end_line":13,"end_character":46},"in_reply_to":"df140735_804861cc","updated":"2017-05-30 21:34:57.000000000","message":"Done","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"6460e7982619ccc756f372a558b1ab672033ed5d","unresolved":false,"context_lines":[{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I9ba309a0889f9b8fb4902203e9689218974eb463"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"df140735_401489a0","line":17,"updated":"2017-05-30 20:20:45.000000000","message":"can you link the patch to the blueprint that implements the new ingress rule?","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"99cd6b405ed2ce14a4fd1b355cd39312f0125394","unresolved":false,"context_lines":[{"line_number":14,"context_line":"additional attribute"},{"line_number":15,"context_line":"ingress\u003dTrue"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I9ba309a0889f9b8fb4902203e9689218974eb463"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"df140735_36bc3fdb","line":17,"in_reply_to":"df140735_401489a0","updated":"2017-05-30 21:34:57.000000000","message":"Done","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e0d0f85d1360fd3024ea19182508980b11c1f24b","unresolved":false,"context_lines":[{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules like e.g. bandwidth limit rule type with"},{"line_number":11,"context_line":"ingress direction introduced in blueprint"},{"line_number":12,"context_line":"https://blueprints.launchpad.net/neutron/+spec/instance-ingress-bw-limit"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":15,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"df140735_362b1fec","line":12,"updated":"2017-05-30 21:41:25.000000000","message":"This is usually done as:\n\nPartially-Implements: blueprint ...","commit_id":"bacdfd296941761aaaab56ecfa491c976de993dd"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"c703f78833cbb2707b0962309c29579fdaeda40a","unresolved":false,"context_lines":[{"line_number":9,"context_line":"There is a need to extend QoS L2 drivers interface to allow"},{"line_number":10,"context_line":"remove of ingress rules like e.g. bandwidth limit rule type with"},{"line_number":11,"context_line":"ingress direction introduced in blueprint"},{"line_number":12,"context_line":"https://blueprints.launchpad.net/neutron/+spec/instance-ingress-bw-limit"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":15,"context_line":"L2 drivers should provide handler method witch accepts"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"df140735_d63f43d4","line":12,"in_reply_to":"df140735_362b1fec","updated":"2017-05-30 21:52:34.000000000","message":"changed. Sorry, I misunderstood You","commit_id":"bacdfd296941761aaaab56ecfa491c976de993dd"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e0d0f85d1360fd3024ea19182508980b11c1f24b","unresolved":false,"context_lines":[{"line_number":12,"context_line":"https://blueprints.launchpad.net/neutron/+spec/instance-ingress-bw-limit"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":15,"context_line":"L2 drivers should provide handler method witch accepts"},{"line_number":16,"context_line":"additional attribute"},{"line_number":17,"context_line":"ingress\u003dTrue"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"df140735_d6398341","line":15,"range":{"start_line":15,"start_character":41,"end_line":15,"end_character":46},"updated":"2017-05-30 21:41:25.000000000","message":"which","commit_id":"bacdfd296941761aaaab56ecfa491c976de993dd"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"c703f78833cbb2707b0962309c29579fdaeda40a","unresolved":false,"context_lines":[{"line_number":12,"context_line":"https://blueprints.launchpad.net/neutron/+spec/instance-ingress-bw-limit"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"For that kind of rule types which have direction attribute"},{"line_number":15,"context_line":"L2 drivers should provide handler method witch accepts"},{"line_number":16,"context_line":"additional attribute"},{"line_number":17,"context_line":"ingress\u003dTrue"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"df140735_562b3393","line":15,"range":{"start_line":15,"start_character":41,"end_line":15,"end_character":46},"in_reply_to":"df140735_d6398341","updated":"2017-05-30 21:52:34.000000000","message":"fixed, I don\u0027t know how I missed this one earlier :)","commit_id":"bacdfd296941761aaaab56ecfa491c976de993dd"}],"neutron/agent/l2/extensions/qos.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"0259f7b900d3146898ac12f394e94fb4492a00e7","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff0f0b1f_3ee366db","line":56,"range":{"start_line":54,"start_character":6,"end_line":56,"end_character":27},"updated":"2017-05-24 09:41:34.000000000","message":"With this change, you need to change all ml2 driver qos extension calls. E.g.: https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/drivers/linuxbridge/agent/extension_drivers/qos_driver.py#L69\n\nYou need to change the function names in LB, SRIOV and OVS, for max, min and dscp rule types.","commit_id":"6fae54b094146cca38e0803ded2e7399a1a7f77b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"d35a1b3999f6eba94e58f18bf5fad6eeebdfa211","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff0f0b1f_b5cf8c27","line":56,"range":{"start_line":54,"start_character":6,"end_line":56,"end_character":27},"in_reply_to":"ff0f0b1f_3ee366db","updated":"2017-05-24 14:23:48.000000000","message":"No, old names can be named as are now. Those new methods should be added to each driver probably.","commit_id":"6fae54b094146cca38e0803ded2e7399a1a7f77b"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"bd1e1dca15b55c4d62a30dfd120dc83aa01a1ee1","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_a8a96390","line":56,"range":{"start_line":52,"start_character":0,"end_line":56,"end_character":27},"updated":"2017-05-24 22:22:06.000000000","message":"I would change this to:\n\nFor ingress rules, handlers with the following names must exist:\n\ncreate_\u003ctype\u003e_ingress\nupdate_\u003ctype\u003e_ingress\ndelete_\u003ctype\u003e_ingress","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"acd1bff7b5b17605c87bde7a33713195463bbfea","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_77f30775","line":56,"range":{"start_line":52,"start_character":0,"end_line":56,"end_character":27},"in_reply_to":"ff0f0b1f_2876733f","updated":"2017-05-25 21:53:31.000000000","message":"it\u0027s not necessary but I wanted to do it to be more \"consistent\". But probably You are right and I will left only delete_\u003ctype\u003e_ingress method","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"fe4d73b25c374774ee529f9b7dbbcc1643065ac5","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_2876733f","line":56,"range":{"start_line":52,"start_character":0,"end_line":56,"end_character":27},"in_reply_to":"ff0f0b1f_a8a96390","updated":"2017-05-24 22:23:14.000000000","message":"Do we need to change the naming convention for create and update?","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"acd1bff7b5b17605c87bde7a33713195463bbfea","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for bandwidth_limit rule for ingress"},{"line_number":53,"context_line":"    # direction. For this rule handler has following names:"},{"line_number":54,"context_line":"    # create_ingress_\u003ctype\u003e"},{"line_number":55,"context_line":"    # update_ingress_\u003ctype\u003e"},{"line_number":56,"context_line":"    # delete_ingress_\u003ctype\u003e"},{"line_number":57,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_77cae71a","line":56,"range":{"start_line":52,"start_character":0,"end_line":56,"end_character":27},"in_reply_to":"ff0f0b1f_a8a96390","updated":"2017-05-25 21:53:31.000000000","message":"done","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"bd1e1dca15b55c4d62a30dfd120dc83aa01a1ee1","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        :param port: port object."},{"line_number":100,"context_line":"        :param qos_policy: the QoS policy to be removed from port."},{"line_number":101,"context_line":"        \"\"\""},{"line_number":102,"context_line":"        if qos_policy is None:"},{"line_number":103,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":104,"context_line":"                self._handle_rule_delete(port, rule_type\u003drule_type)"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"},{"line_number":107,"context_line":"                self._handle_rule_delete(port, rule\u003drule)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def _iterate_rules(self, rules):"},{"line_number":110,"context_line":"        for rule in rules:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_a86e03c6","line":107,"range":{"start_line":102,"start_character":0,"end_line":107,"end_character":57},"updated":"2017-05-24 22:22:06.000000000","message":"Here\u0027s what I had in mind for this method:\n\n        if qos_policy is None:\n              for rule_type in self.SUPPORTED_RULES:\n                  self._handle_rule_delete(port, rule_type)\n                  if self._rule_type_has_ingress_direction(rule_type):\n                      self._handle_rule_delete(port, rule_type, True)\n          else:\n              for rule in self._iterate_rules(qos_policy.rules):\n                  is_ingress \u003d (rule.direction \u003d\u003d constants.INGRESS_DIRECTION)\n                  self._handle_rule_delete(port, rule.rule_type, is_ingress)\n\n\nThis assumes that we define the following:\n\n    def _rule_type_has_ingress_direction(self, rule_type):\n          rules \u003d self.SUPPORTED_RULES\n          return (constants.INGRESS_DIRECTION in\n                  rules[rule_type][qos_consts.DIRECTION][\u0027type:values\u0027])","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"acd1bff7b5b17605c87bde7a33713195463bbfea","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        :param port: port object."},{"line_number":100,"context_line":"        :param qos_policy: the QoS policy to be removed from port."},{"line_number":101,"context_line":"        \"\"\""},{"line_number":102,"context_line":"        if qos_policy is None:"},{"line_number":103,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":104,"context_line":"                self._handle_rule_delete(port, rule_type\u003drule_type)"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"},{"line_number":107,"context_line":"                self._handle_rule_delete(port, rule\u003drule)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def _iterate_rules(self, rules):"},{"line_number":110,"context_line":"        for rule in rules:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_fa649ee7","line":107,"range":{"start_line":102,"start_character":0,"end_line":107,"end_character":57},"in_reply_to":"ff0f0b1f_a86e03c6","updated":"2017-05-25 21:53:31.000000000","message":"Done","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"bd1e1dca15b55c4d62a30dfd120dc83aa01a1ee1","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                            {\u0027rule_id\u0027: rule.id, \u0027rule_type\u0027: rule_type})"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def _handle_rule_delete(self, port, rule_type\u003dNone, rule\u003dNone):"},{"line_number":120,"context_line":"        if rule:"},{"line_number":121,"context_line":"            handler_name \u003d self._get_handler_for_rule(\"delete\", rule)"},{"line_number":122,"context_line":"        else:"},{"line_number":123,"context_line":"            handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":124,"context_line":"            if rule_type \u003d\u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT:"},{"line_number":125,"context_line":"                # NOTE(slaweq): if rule_type was given instead of rule, we can"},{"line_number":126,"context_line":"                # assume that there was no qos_policy given and all rules"},{"line_number":127,"context_line":"                # should be cleared. In such case for RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":128,"context_line":"                # we should also call handler method to delete ingress"},{"line_number":129,"context_line":"                # bandwidth limit rule"},{"line_number":130,"context_line":"                ingress_bw_limit_handler_name \u003d \"delete_%s_%s\" % ("},{"line_number":131,"context_line":"                    constants.INGRESS_DIRECTION,"},{"line_number":132,"context_line":"                    qos_consts.RULE_TYPE_BANDWIDTH_LIMIT)"},{"line_number":133,"context_line":"                if hasattr(self, ingress_bw_limit_handler_name):"},{"line_number":134,"context_line":"                    ingress_bw_limit_handler \u003d getattr("},{"line_number":135,"context_line":"                        self, ingress_bw_limit_handler_name)"},{"line_number":136,"context_line":"                    ingress_bw_limit_handler(port)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"        handler \u003d getattr(self, handler_name)"},{"line_number":139,"context_line":"        handler(port)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_c8eed720","line":136,"range":{"start_line":120,"start_character":0,"end_line":136,"end_character":50},"updated":"2017-05-24 22:22:06.000000000","message":"Based on what I propose in L102-107 above, this is what this method would look like:\n\n    def _handle_rule_delete(self, port, rule_type, is_ingress\u003dFalse):\n          handler_name \u003d \"\".join((\"delete_\", rule_type))\n          if is_ingress:\n              handler_name \u003d \"%s_%s\" % (handler_name,\n                                        constants.INGRESS_DIRECTION)\n          handler \u003d getattr(self, handler_name)\n          handler(port)\n\nPlease note that I am taking a slightly different approach to naming: I am adding \"_ingress\" at the end of the method, for simplicity\u0027s sake","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"acd1bff7b5b17605c87bde7a33713195463bbfea","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                            {\u0027rule_id\u0027: rule.id, \u0027rule_type\u0027: rule_type})"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def _handle_rule_delete(self, port, rule_type\u003dNone, rule\u003dNone):"},{"line_number":120,"context_line":"        if rule:"},{"line_number":121,"context_line":"            handler_name \u003d self._get_handler_for_rule(\"delete\", rule)"},{"line_number":122,"context_line":"        else:"},{"line_number":123,"context_line":"            handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":124,"context_line":"            if rule_type \u003d\u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT:"},{"line_number":125,"context_line":"                # NOTE(slaweq): if rule_type was given instead of rule, we can"},{"line_number":126,"context_line":"                # assume that there was no qos_policy given and all rules"},{"line_number":127,"context_line":"                # should be cleared. In such case for RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":128,"context_line":"                # we should also call handler method to delete ingress"},{"line_number":129,"context_line":"                # bandwidth limit rule"},{"line_number":130,"context_line":"                ingress_bw_limit_handler_name \u003d \"delete_%s_%s\" % ("},{"line_number":131,"context_line":"                    constants.INGRESS_DIRECTION,"},{"line_number":132,"context_line":"                    qos_consts.RULE_TYPE_BANDWIDTH_LIMIT)"},{"line_number":133,"context_line":"                if hasattr(self, ingress_bw_limit_handler_name):"},{"line_number":134,"context_line":"                    ingress_bw_limit_handler \u003d getattr("},{"line_number":135,"context_line":"                        self, ingress_bw_limit_handler_name)"},{"line_number":136,"context_line":"                    ingress_bw_limit_handler(port)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"        handler \u003d getattr(self, handler_name)"},{"line_number":139,"context_line":"        handler(port)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_da61dad5","line":136,"range":{"start_line":120,"start_character":0,"end_line":136,"end_character":50},"in_reply_to":"ff0f0b1f_c8eed720","updated":"2017-05-25 21:53:31.000000000","message":"Done","commit_id":"57f5e6c10374938f76ca9497dd0d9c5e73e75f1c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"ebaf397893eb5736630a45ec2efe9c164af153fb","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_28019eb0","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"updated":"2017-05-26 09:01:31.000000000","message":"This implementation, in general, is very specific only for bw limit and ingress rules. As you said, the naming convention is inconsistent. But this is just an opinion, nothing else.","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"a211594e661dfe63fcbb0780fe1d8eb13463d4d2","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"df140735_be403c09","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"df140735_0aa003f4","updated":"2017-05-30 08:35:34.000000000","message":"IMO \"is_ingress\u003dTrue\" should be fine as it\u0027s only required for ingress direction. In other cases (for backward compatibility) we are not sending any parameter to handler method.\nI changed it in PS 6. Please take a look again on it :)","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"abf86e12aa92fc6bf0047e275ab934b8f5f2d1af","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"df140735_58913d99","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"df140735_3c2447c6","updated":"2017-05-29 21:43:30.000000000","message":"so You propose to send \"is_ingress\u003dTrue\" as parameter to driver\u0027s delete_\u003ctype\u003e method if rule_type supports ingress direction, right?","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"9971b5ab663b78edd1e2fe6c3258a4ed5a728b07","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"df140735_0aa003f4","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"df140735_58913d99","updated":"2017-05-30 07:40:21.000000000","message":"Yes, but if the driver supports BOTH directions. If only one is supported (egress is the only one now supported), this is irrelevant.\n\nSo yes, if the driver supports BOTH directions, send \"direction\u003dEGRESS\" or \"direction\u003dINGRESS\"\n\nThe parameter \"is_ingress\u003dTrue\", again, is too specific for ingress (in my opinion).\n\nPlease, contact me in IRC if you want to talk about this point.\n\nThank you Slaweq!","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b7ee5da7b7ca78a42cb03c28025c846cfaa990fa","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"df140735_fed23434","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"df140735_be403c09","updated":"2017-05-30 08:42:03.000000000","message":"I know, I was proposing just sending the \"direction\" parameter only when the rule has both, instead of sending \"is_ingress\"\n\nBut this is just a nit, nothing important. Is just I prefer to have more generic input parameters.","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"22b71056446ff6927a8ccde8cd2dff991667a8df","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"df140735_3c2447c6","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"ff0f0b1f_075fd5e5","updated":"2017-05-29 12:38:50.000000000","message":"OK, that\u0027s a possible solution to avoid having this specific name for delete_ingress.\n\nSteps:\n1) In this patch, check is the driver for this specific rule, supports both directions. Currently only egress is supported, but LB and OVS are going to implement ingress. If this driver for this rule supports it, send the direction in the \"delete\" function call.\n2) In following patches implementing ingress direction, implement \"delete\" function with \"rule_direction\" as a parameter.\n\nWhat do you think?","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"7d6c0a4fde821a7777232633756d4d688725dd7a","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_a02b772e","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"ff0f0b1f_28019eb0","updated":"2017-05-26 17:28:56.000000000","message":"I agree that in the way this comment is stated, it is very specific for bw limit and ingress rules. In fact, in my previous review, I proposed to change L52-54 to something similar to this:\n\n\"For deletion of ingress rules, handlers with the following names must exist:\n\ndelete_\u003ctype\u003e_ingress\"\n\nThis accounts for the fact that in the future, ingress rules for other types will have to be handled.\n\nHaving said this, I am completely open to other alternatives, as long as we don\u0027t break the API for implementations that already exist. Here\u0027s my understanding of the situation:\n\nFor deletion of ingress rules, we cannot call a generic delete_\u003ctype\u003e method. We either add a rule argument to existing and future delete_\u003ctype\u003e methods or we call a specific delete_\u003ctype\u003e_ingress method. The former breaks the API of existing QoS implementations, so that is why I have advocated the latter approach, which allows us to extend the API defined here without breaking existing implementations. We are trying to come up with a trade-off between keeping a naming convention and extending the API definition so we can add new functionality that wasn\u0027t completely foreseen when the naming convention was originally defined.\n\nWhat alternative can we come up with?","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"af542fe200d035c02dabf022d0c55fcec4aeceb5","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_075fd5e5","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"ff0f0b1f_71465324","updated":"2017-05-26 20:11:43.000000000","message":"I don\u0027t know about other solution to change that without breaking 3rd part drivers. I just made it more \"generic\" for all type of rules with direction\u003dingress","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"6379128f8877b9f2c184155f070cd8b25be00087","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this rule for deletion of bandwidth_limit rule"},{"line_number":53,"context_line":"    # for ingress direction. For this rule delete handler has following name:"},{"line_number":54,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":55,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_71465324","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":27},"in_reply_to":"ff0f0b1f_a02b772e","updated":"2017-05-26 18:42:06.000000000","message":"Furthermore, is my statement of the situation in the previous comment correct? Can we re-state the situation in such a way that we can get away with not having to add arguments to delete_\u003ctype\u003e methods, not modifying this API definition and still be able to handle deletion of ingress rules?","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"ebaf397893eb5736630a45ec2efe9c164af153fb","unresolved":false,"context_lines":[{"line_number":137,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":140,"context_line":"        return (rule.rule_type \u003d\u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT and"},{"line_number":141,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_8812ea08","line":140,"range":{"start_line":140,"start_character":16,"end_line":140,"end_character":70},"updated":"2017-05-26 09:01:31.000000000","message":"Why this condition? I know currently only bw-limit has both directions implemented. But in a future new rules could implement ingress direction. Is this condition needed?","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"7d6c0a4fde821a7777232633756d4d688725dd7a","unresolved":false,"context_lines":[{"line_number":137,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":140,"context_line":"        return (rule.rule_type \u003d\u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT and"},{"line_number":141,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_e0504f0d","line":140,"range":{"start_line":140,"start_character":16,"end_line":140,"end_character":70},"in_reply_to":"ff0f0b1f_8812ea08","updated":"2017-05-26 17:28:56.000000000","message":"I agree, this condition shouldn\u0027t be here. In fact, in the code snippets I proposed in my previous review, the only check is:\n\nrule.direction \u003d\u003d constants.INGRESS_DIRECTION","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"af542fe200d035c02dabf022d0c55fcec4aeceb5","unresolved":false,"context_lines":[{"line_number":137,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":140,"context_line":"        return (rule.rule_type \u003d\u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT and"},{"line_number":141,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ff0f0b1f_c7267d69","line":140,"range":{"start_line":140,"start_character":16,"end_line":140,"end_character":70},"in_reply_to":"ff0f0b1f_e0504f0d","updated":"2017-05-26 20:11:43.000000000","message":"Done","commit_id":"7421fe0fddd85f65a8f4ad9b3c0bd122510083e0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8ffe46c485ab173442a129bedc31fd4c60c6c94e","unresolved":false,"context_lines":[{"line_number":42,"context_line":"    \"\"\""},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # Each QoS driver should define the set of rule types that it supports, and"},{"line_number":45,"context_line":"    # corresponding handlers that usually has the following names:"},{"line_number":46,"context_line":"    #"},{"line_number":47,"context_line":"    # create_\u003ctype\u003e"},{"line_number":48,"context_line":"    # update_\u003ctype\u003e"}],"source_content_type":"text/x-python","patch_set":6,"id":"df140735_838e3876","line":45,"range":{"start_line":45,"start_character":34,"end_line":45,"end_character":41},"updated":"2017-05-30 13:56:06.000000000","message":"This is the reason for the -1. Since this is an API definition, docstrings and comments should be precise. With the approach proposed in this revision, we are not changing the methods naming convention. Therefore, \u0027usually\u0027 should be removed from here.","commit_id":"9786aa795d9ce8d5854e3bc3bb140562d03e2f5d"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"d8ac07007d0ee4cc016e3d7c0f3270561af0f8bd","unresolved":false,"context_lines":[{"line_number":42,"context_line":"    \"\"\""},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # Each QoS driver should define the set of rule types that it supports, and"},{"line_number":45,"context_line":"    # corresponding handlers that usually has the following names:"},{"line_number":46,"context_line":"    #"},{"line_number":47,"context_line":"    # create_\u003ctype\u003e"},{"line_number":48,"context_line":"    # update_\u003ctype\u003e"}],"source_content_type":"text/x-python","patch_set":6,"id":"df140735_46526e15","line":45,"range":{"start_line":45,"start_character":34,"end_line":45,"end_character":41},"in_reply_to":"df140735_838e3876","updated":"2017-05-30 14:17:25.000000000","message":"Done","commit_id":"9786aa795d9ce8d5854e3bc3bb140562d03e2f5d"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"e4515661b18b73c13ad22c1dd6c92963032598d4","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        if qos_policy is None:"},{"line_number":93,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":94,"context_line":"                self._handle_rule_delete(port, rule_type)"},{"line_number":95,"context_line":"                if self._rule_type_has_ingress_direction(rule_type):"},{"line_number":96,"context_line":"                    self._handle_rule_delete(port, rule_type, True)"},{"line_number":97,"context_line":"        else:"},{"line_number":98,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_126dd003","line":95,"updated":"2017-05-30 16:25:37.000000000","message":"Question: I wonder why we need to treat \"ingress\" differently. Don\u0027t we need a similar check for egress direction?","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cb85e0d035f98ed5eb975e24d965014eb8583657","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        if qos_policy is None:"},{"line_number":93,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":94,"context_line":"                self._handle_rule_delete(port, rule_type)"},{"line_number":95,"context_line":"                if self._rule_type_has_ingress_direction(rule_type):"},{"line_number":96,"context_line":"                    self._handle_rule_delete(port, rule_type, True)"},{"line_number":97,"context_line":"        else:"},{"line_number":98,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_3d8f852b","line":95,"in_reply_to":"df140735_126dd003","updated":"2017-05-30 16:46:34.000000000","message":"Currently there are only:\n* rules without direction, all are egress\n* rules with direction, and all of them have only egress direction enable\n\nBecause we need to keep backwards compatibility, only ingress rules (now there are no ingress rules implemented) will have this parameter","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"dd6938bb975395bda81468d1e294455e9436daf8","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        if qos_policy is None:"},{"line_number":93,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":94,"context_line":"                self._handle_rule_delete(port, rule_type)"},{"line_number":95,"context_line":"                if self._rule_type_has_ingress_direction(rule_type):"},{"line_number":96,"context_line":"                    self._handle_rule_delete(port, rule_type, True)"},{"line_number":97,"context_line":"        else:"},{"line_number":98,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_ccb1f891","line":95,"in_reply_to":"df140735_3d8f852b","updated":"2017-05-30 18:39:53.000000000","message":"Egress is handled with \"default\" (existing) handler. Please see my comment inhttps://review.openstack.org/#/c/466899/7//COMMIT_MSG","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"e4515661b18b73c13ad22c1dd6c92963032598d4","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":139,"context_line":"        return (hasattr(rule, \"direction\") and"},{"line_number":140,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"class PortPolicyMap(object):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_d2275812","line":140,"range":{"start_line":139,"start_character":16,"end_line":140,"end_character":61},"updated":"2017-05-30 16:25:37.000000000","message":"Could you use getattr() rather than hasattr()?\n\nI think this is a good writing on hasattr()\nhttps://hynek.me/articles/hasattr/","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cb85e0d035f98ed5eb975e24d965014eb8583657","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":139,"context_line":"        return (hasattr(rule, \"direction\") and"},{"line_number":140,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"class PortPolicyMap(object):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_9d77b13b","line":140,"range":{"start_line":139,"start_character":16,"end_line":140,"end_character":61},"in_reply_to":"df140735_d2275812","updated":"2017-05-30 16:46:34.000000000","message":"+1 agree","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"dd6938bb975395bda81468d1e294455e9436daf8","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":139,"context_line":"        return (hasattr(rule, \"direction\") and"},{"line_number":140,"context_line":"                rule.direction \u003d\u003d constants.INGRESS_DIRECTION)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"class PortPolicyMap(object):"}],"source_content_type":"text/x-python","patch_set":7,"id":"df140735_821ce96a","line":140,"range":{"start_line":139,"start_character":16,"end_line":140,"end_character":61},"in_reply_to":"df140735_d2275812","updated":"2017-05-30 18:39:53.000000000","message":"Done","commit_id":"87419571010f0df14cd69b0fd9a6ae15b20dda80"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e0dcdb6de3004c73563b3e3ef15ceb19e8b04c92","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # For deletion of rule type which supports direction\u003dingress additional"},{"line_number":53,"context_line":"    # parameter \"ingress\u003dTrue\" is passed to handler."},{"line_number":54,"context_line":"    SUPPORTED_RULES \u003d set()"},{"line_number":55,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"df140735_160b7b8e","line":52,"updated":"2017-05-30 21:40:35.000000000","message":"I find this very weird that you need to special case it this way. I would suggest to introduce a new delete_bwlimit_ingress_rule, that would be more explicit. But that\u0027s a matter of taste I guess?","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"6460e7982619ccc756f372a558b1ab672033ed5d","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":115,"context_line":"        handler \u003d getattr(self, handler_name)"},{"line_number":116,"context_line":"        if is_ingress:"},{"line_number":117,"context_line":"            handler(port, ingress\u003dis_ingress)"},{"line_number":118,"context_line":"        else:"},{"line_number":119,"context_line":"            handler(port)"},{"line_number":120,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"df140735_802181fe","line":117,"updated":"2017-05-30 20:20:45.000000000","message":"I don\u0027t think that all existing handlers are ready to accept the new argument. This is a breaking change it seems.\n\nThe fact that it doesn\u0027t break a single test case is also concerning. I think we should work out test coverage for the code before touching the code.","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"99cd6b405ed2ce14a4fd1b355cd39312f0125394","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":115,"context_line":"        handler \u003d getattr(self, handler_name)"},{"line_number":116,"context_line":"        if is_ingress:"},{"line_number":117,"context_line":"            handler(port, ingress\u003dis_ingress)"},{"line_number":118,"context_line":"        else:"},{"line_number":119,"context_line":"            handler(port)"},{"line_number":120,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"df140735_f60ec7e3","line":117,"in_reply_to":"df140735_802181fe","updated":"2017-05-30 21:34:57.000000000","message":"each driver which will implement support for rule with ingress direction has to support it, otherwise such driver will not work properly.\nIf driver not supports ingress direction (as it is in state of this patch) then this \"ingress\" attribute is not passed to handler.\nIMO it\u0027s not breaking change and the fact that current tests are not failing is proof for that because there is currently no driver which would support ingress direction.","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e0dcdb6de3004c73563b3e3ef15ceb19e8b04c92","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":115,"context_line":"        handler \u003d getattr(self, handler_name)"},{"line_number":116,"context_line":"        if is_ingress:"},{"line_number":117,"context_line":"            handler(port, ingress\u003dis_ingress)"},{"line_number":118,"context_line":"        else:"},{"line_number":119,"context_line":"            handler(port)"},{"line_number":120,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"df140735_b6cbaf51","line":117,"in_reply_to":"df140735_f60ec7e3","updated":"2017-05-30 21:40:35.000000000","message":"Oh ok, I missed the fact that you inspect supported_rules in line 131+. I am not totally sure that overloading existing methods for two goals (ingress and egress) is the best (I would introduce a new entry point for ingress) but that\u0027s probably a matter of taste.","commit_id":"3e1a42447637168838d9c9b8b2eafca1751b0de1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1048d038d3fc46d5dd429d4ae7125c47b96c9b00","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this tule for deletion of rules with"},{"line_number":53,"context_line":"    # attribute direction set to ingress (e.g. bandwidth limit rule)."},{"line_number":54,"context_line":"    # For deletion of such rule typys delete handler has following name:"},{"line_number":55,"context_line":"    # delete_\u003ctype\u003e_ingress"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_e881a8c4","line":52,"range":{"start_line":52,"start_character":36,"end_line":52,"end_character":40},"updated":"2017-06-05 16:42:53.000000000","message":"rule","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f11b420500c66ec163c5856778d90d2977c0beb3","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    # delete_\u003ctype\u003e"},{"line_number":50,"context_line":"    #"},{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this tule for deletion of rules with"},{"line_number":53,"context_line":"    # attribute direction set to ingress (e.g. bandwidth limit rule)."},{"line_number":54,"context_line":"    # For deletion of such rule typys delete handler has following name:"},{"line_number":55,"context_line":"    # delete_\u003ctype\u003e_ingress"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_abb16a49","line":52,"range":{"start_line":52,"start_character":36,"end_line":52,"end_character":40},"in_reply_to":"df140735_e881a8c4","updated":"2017-06-05 17:00:20.000000000","message":"Done","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1048d038d3fc46d5dd429d4ae7125c47b96c9b00","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this tule for deletion of rules with"},{"line_number":53,"context_line":"    # attribute direction set to ingress (e.g. bandwidth limit rule)."},{"line_number":54,"context_line":"    # For deletion of such rule typys delete handler has following name:"},{"line_number":55,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_a86f3029","line":54,"range":{"start_line":54,"start_character":32,"end_line":54,"end_character":38},"updated":"2017-06-05 16:42:53.000000000","message":"types?","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f11b420500c66ec163c5856778d90d2977c0beb3","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    # where \u003ctype\u003e is one of VALID_RULE_TYPES"},{"line_number":52,"context_line":"    # There is exception from this tule for deletion of rules with"},{"line_number":53,"context_line":"    # attribute direction set to ingress (e.g. bandwidth limit rule)."},{"line_number":54,"context_line":"    # For deletion of such rule typys delete handler has following name:"},{"line_number":55,"context_line":"    # delete_\u003ctype\u003e_ingress"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_8bac66ed","line":54,"range":{"start_line":54,"start_character":32,"end_line":54,"end_character":38},"in_reply_to":"df140735_a86f3029","updated":"2017-06-05 17:00:20.000000000","message":"Done","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1048d038d3fc46d5dd429d4ae7125c47b96c9b00","unresolved":false,"context_lines":[{"line_number":94,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":95,"context_line":"                self._handle_rule_delete(port, rule_type)"},{"line_number":96,"context_line":"                if self._rule_type_has_ingress_direction(rule_type):"},{"line_number":97,"context_line":"                    self._handle_rule_delete(port, rule_type, True)"},{"line_number":98,"context_line":"        else:"},{"line_number":99,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"},{"line_number":100,"context_line":"                self._handle_rule_delete("}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_08cdfc2f","line":97,"range":{"start_line":97,"start_character":62,"end_line":97,"end_character":67},"updated":"2017-06-05 16:42:53.000000000","message":"for clarity, please specify keyword name as in is_ingress\u003dTrue","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f11b420500c66ec163c5856778d90d2977c0beb3","unresolved":false,"context_lines":[{"line_number":94,"context_line":"            for rule_type in self.SUPPORTED_RULES:"},{"line_number":95,"context_line":"                self._handle_rule_delete(port, rule_type)"},{"line_number":96,"context_line":"                if self._rule_type_has_ingress_direction(rule_type):"},{"line_number":97,"context_line":"                    self._handle_rule_delete(port, rule_type, True)"},{"line_number":98,"context_line":"        else:"},{"line_number":99,"context_line":"            for rule in self._iterate_rules(qos_policy.rules):"},{"line_number":100,"context_line":"                self._handle_rule_delete("}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_2b879a64","line":97,"range":{"start_line":97,"start_character":62,"end_line":97,"end_character":67},"in_reply_to":"df140735_08cdfc2f","updated":"2017-06-05 17:00:20.000000000","message":"Done","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1048d038d3fc46d5dd429d4ae7125c47b96c9b00","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                                \u0027%(rule_type)s; skipping\u0027),"},{"line_number":112,"context_line":"                            {\u0027rule_id\u0027: rule.id, \u0027rule_type\u0027: rule_type})"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def _handle_rule_delete(self, port, rule_type, is_ingress\u003dFalse):"},{"line_number":115,"context_line":"        handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":116,"context_line":"        if is_ingress:"},{"line_number":117,"context_line":"            handler_name \u003d \"%s_%s\" % (handler_name,"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_c8eb8475","line":114,"range":{"start_line":114,"start_character":51,"end_line":114,"end_character":61},"updated":"2017-06-05 16:42:53.000000000","message":"nit: maybe just ingress\u003dFalse","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f11b420500c66ec163c5856778d90d2977c0beb3","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                                \u0027%(rule_type)s; skipping\u0027),"},{"line_number":112,"context_line":"                            {\u0027rule_id\u0027: rule.id, \u0027rule_type\u0027: rule_type})"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def _handle_rule_delete(self, port, rule_type, is_ingress\u003dFalse):"},{"line_number":115,"context_line":"        handler_name \u003d \"\".join((\"delete_\", rule_type))"},{"line_number":116,"context_line":"        if is_ingress:"},{"line_number":117,"context_line":"            handler_name \u003d \"%s_%s\" % (handler_name,"}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_ab9a8ab5","line":114,"range":{"start_line":114,"start_character":51,"end_line":114,"end_character":61},"in_reply_to":"df140735_c8eb8475","updated":"2017-06-05 17:00:20.000000000","message":"Done","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1048d038d3fc46d5dd429d4ae7125c47b96c9b00","unresolved":false,"context_lines":[{"line_number":137,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":140,"context_line":"        rule_direction \u003d getattr(rule, \"direction\", None)"},{"line_number":141,"context_line":"        return rule_direction \u003d\u003d constants.INGRESS_DIRECTION"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_e8b6488a","line":140,"range":{"start_line":140,"start_character":52,"end_line":140,"end_character":56},"updated":"2017-06-05 16:42:53.000000000","message":"(No action required) maybe constants.EGRESS_DIRECTION, to be slightly more explicit about the default?","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f11b420500c66ec163c5856778d90d2977c0beb3","unresolved":false,"context_lines":[{"line_number":137,"context_line":"                supported_rule[qos_consts.DIRECTION][\u0027type:values\u0027])"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _rule_is_ingress_direction(self, rule):"},{"line_number":140,"context_line":"        rule_direction \u003d getattr(rule, \"direction\", None)"},{"line_number":141,"context_line":"        return rule_direction \u003d\u003d constants.INGRESS_DIRECTION"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"df140735_4b8bee66","line":140,"range":{"start_line":140,"start_character":52,"end_line":140,"end_character":56},"in_reply_to":"df140735_e8b6488a","updated":"2017-06-05 17:00:20.000000000","message":"Done","commit_id":"d7aacc98288f60baf4009c7cd47c9214a26d8553"}]}