)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9531,"name":"liuyulong","display_name":"LIU Yulong","email":"i@liuyulong.me","username":"LIU-Yulong"},"change_message_id":"b024b99e2d9f6a2ac0dde3093864f8d30384122e","unresolved":true,"context_lines":[{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Port list and show speed-up is ~10-25%."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Depends on neutron-lib change:"},{"line_number":15,"context_line":"https://review.opendev.org/c/openstack/neutron-lib/+/788729"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I4bfcac16a54b766e0dc97f58ba4048eb709fa88f"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7bae00bf_a79dcc4b","line":15,"range":{"start_line":14,"start_character":0,"end_line":15,"end_character":59},"updated":"2021-07-23 06:17:22.000000000","message":"This is not needed now, neutron-lib with this change had beed added to 2.12.0 and neutron requirements set this version as well.\n\nhttps://review.opendev.org/c/openstack/neutron/+/796404","commit_id":"0af8497076d58d54d996c70a30e3b5a24e45b3af"}],"neutron/db/db_base_plugin_v2.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"501d105aee709f3a16827ad180547c2ca9697057","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"0b877700_9e784b67","line":1527,"updated":"2021-05-25 17:11:01.000000000","message":"as commented in [1], those fields are not part of the port SQL query","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"05423fba0cc5976d0f62770f895d753c2194b6fc","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"0d7c76e6_d60fa1c4","line":1527,"in_reply_to":"04fa2756_6ad93763","updated":"2021-06-24 10:14:01.000000000","message":"\u003e The queries you have reported in both links are the same, there is no difference. Those parameters are already loaded in another query. With this patch you are lazy loading them in another DB transaction, if needed.\n\nNot sure I understand which another transaction you mean, can you please clarify?\nI see 1 read transaction and it includes 1 query for ports table and 7 subqueries (http://paste.openstack.org/show/805749/). After the patch it\u0027s same query for ports table and only 4 subqueries (http://paste.openstack.org/show/805748/). My tests showed speedup in both list and show operations because of eliminating those 3 subqueries.\n\n\u003e \n\u003e My reason to give a -1 to this patch is that if we are going to change both in show and list commands the port OVO model (because this is what we are doing), instead of doing that dynamically, we can do it statically changing the relationship of those fields (port_forwardings, binding_levels, distributed_port_binding).\n\nThe reason of doing it dynamically for List and Show is that for other operations (like port update/bind) those fields could be more effective to be loaded in current (subquery) mode. If do it statically we will likely slowdown those operations, won\u0027t we?\n\n\u003e \n\u003e Just an example, for those parameters we already changed the port OVO view model [1]. The loading method was changed from \"joined\" to \"subquery\". We can change it again to \"select\" (same as lazyloading [2]).\n\u003e \n\u003e I\u0027ve tested that and I see a little speed improvement, both listing and showing a port, when changing the relationship of those field to select:\n\u003e - list (250 ports): 1.80secs to 1.65secs\n\u003e - show: 0.105secs to 0.090secs\n\u003e \n\u003e Of course that will also depend on the DB backend but I think that could be interesting to propose this patch.\n\u003e \n\u003e [1]https://review.opendev.org/c/openstack/neutron/+/408143\n\u003e [2]https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"af9e392ee92cb4fc4e7f50a1cd76d6ccd3725b9e","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"c248c9d6_b4eeb49d","line":1527,"in_reply_to":"09f5c8b2_2f537899","updated":"2021-06-25 08:28:51.000000000","message":"\u003e Disregard my first comment, once I published it I saw what you said.\n\u003e \n\u003e When a port is updated, \"get_port\" is called too. For example, when you are binding it to a VM. So that change will affect to all port operations too. The effect of changing the relationship statically or dynamically will be the same (and I prefer to have it defined in the DB models).\n\nright, but still there are many places where plugin.get_port() is not used - like ml2.db.get_port() (+ other funcs in ml2.db) used by ml2 plugin. Static change will affect it.\n\n\u003e \n\u003e So I\u0027ll remove my -1 and I\u0027ll ask other reviewers to check it. As I said, I not against this change but changing the DB models statically.","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"36ba0bdf5f75ee1336b06c892631be60eaa002eb","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"0ceaf26f_7a96b0fa","line":1527,"in_reply_to":"0b877700_9e784b67","updated":"2021-05-26 13:03:41.000000000","message":"That\u0027s correct, port SQL query is the same, but the number of subqueries is lower, even for single port subqueries for port_forwardings, binding_levels and distributed_port_binding are done when port is fetched from DB. I\u0027ll put paste links from OSProfiler shortly","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"1f93f23e0a2e5a4cc7471a826785c623a07a3687","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"13571d29_dd04465f","line":1527,"in_reply_to":"0ceaf26f_7a96b0fa","updated":"2021-05-26 13:12:22.000000000","message":"Before patch: http://paste.openstack.org/show/805749/\nWith patch: http://paste.openstack.org/show/805748/","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"0083ac002731cc157c236b98cc4135a0df1f07e9","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"09f5c8b2_2f537899","line":1527,"in_reply_to":"0d7c76e6_d60fa1c4","updated":"2021-06-24 13:17:08.000000000","message":"Disregard my first comment, once I published it I saw what you said.\n\nWhen a port is updated, \"get_port\" is called too. For example, when you are binding it to a VM. So that change will affect to all port operations too. The effect of changing the relationship statically or dynamically will be the same (and I prefer to have it defined in the DB models).\n\nSo I\u0027ll remove my -1 and I\u0027ll ask other reviewers to check it. As I said, I not against this change but changing the DB models statically.","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"573a08ac50c2e3cf9fbe9a0d989a507018421906","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"    @db_api.retry_if_session_inactive()"},{"line_number":1525,"context_line":"    @db_api.CONTEXT_READER"},{"line_number":1526,"context_line":"    def get_port(self, context, id, fields\u003dNone):"},{"line_number":1527,"context_line":"        lazy_fields \u003d [models_v2.Port.port_forwardings,"},{"line_number":1528,"context_line":"                       models_v2.Port.binding_levels,"},{"line_number":1529,"context_line":"                       models_v2.Port.distributed_port_binding]"},{"line_number":1530,"context_line":"        port \u003d self._get_port(context, id, lazy_fields\u003dlazy_fields)"}],"source_content_type":"text/x-python","patch_set":2,"id":"04fa2756_6ad93763","line":1527,"in_reply_to":"13571d29_dd04465f","updated":"2021-06-24 09:50:26.000000000","message":"The queries you have reported in both links are the same, there is no difference. Those parameters are already loaded in another query. With this patch you are lazy loading them in another DB transaction, if needed.\n\nMy reason to give a -1 to this patch is that if we are going to change both in show and list commands the port OVO model (because this is what we are doing), instead of doing that dynamically, we can do it statically changing the relationship of those fields (port_forwardings, binding_levels, distributed_port_binding).\n\nJust an example, for those parameters we already changed the port OVO view model [1]. The loading method was changed from \"joined\" to \"subquery\". We can change it again to \"select\" (same as lazyloading [2]).\n\nI\u0027ve tested that and I see a little speed improvement, both listing and showing a port, when changing the relationship of those field to select:\n- list (250 ports): 1.80secs to 1.65secs\n- show: 0.105secs to 0.090secs\n\nOf course that will also depend on the DB backend but I think that could be interesting to propose this patch.\n\n[1]https://review.opendev.org/c/openstack/neutron/+/408143\n[2]https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html","commit_id":"f7fbd8fa1887792c0458e7b3e2c9f1b36ee8db06"}]}