)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":30615,"name":"Tushar Trambak Gite","email":"tushargite96@gmail.com","username":"tushargite96"},"change_message_id":"0b8c563313eedf64344f40412014d8959462d483","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"f47e5e1b_d0da25ff","updated":"2022-03-24 09:35:10.000000000","message":"LGTM","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"52b448fd5b13a8f8cdfe735fe9f5fe468b59621f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"77270b87_cc144756","updated":"2022-04-11 21:00:21.000000000","message":"We need to get some eyes on the subtransactions\u003dTrue removal in _volume_x_metadata_update().  Otherwise, everything looks OK to me.","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c3482d583189037d3730ac1a8f5ca5dc5719427d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"73539ed3_3c83f380","updated":"2022-04-12 21:38:59.000000000","message":"I think Stephen\u0027s proposal to revisit this issue (after this and the other patches have been merged, and we can test the metadata updates in the context of the new session pattern) makes sense, so upgrading my vote to +2.","commit_id":"0a4a23514117475e437aea58935f86e89d7f1c7a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b66253c7c04cd96b46cc076e757624230480e736","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b79e5b45_559cb76d","updated":"2022-04-12 10:31:35.000000000","message":"I will revisit this TODO once the series is landed. Hopefully my proposed solution is good enough to continue merging this series.","commit_id":"0a4a23514117475e437aea58935f86e89d7f1c7a"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"28afa9a8780bde6aa323cb9489cd5c3a17412d35","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"93ee9d9a_8c2b5f74","updated":"2022-05-24 10:50:04.000000000","message":"LGTM.","commit_id":"0a4a23514117475e437aea58935f86e89d7f1c7a"}],"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"52b448fd5b13a8f8cdfe735fe9f5fe468b59621f","unresolved":true,"context_lines":[{"line_number":3403,"context_line":"    return result"},{"line_number":3404,"context_line":""},{"line_number":3405,"context_line":""},{"line_number":3406,"context_line":"# TODO: We dropped \u0027subtransactions\u003dTrue\u0027 here. Is that an issue?"},{"line_number":3407,"context_line":"def _volume_x_metadata_update("},{"line_number":3408,"context_line":"    context, volume_id, metadata, delete, model, add\u003dTrue, update\u003dTrue"},{"line_number":3409,"context_line":"):"}],"source_content_type":"text/x-python","patch_set":3,"id":"a63feadc_13d38d4e","line":3406,"updated":"2022-04-11 21:00:21.000000000","message":"We\u0027ll have to check with Gorka, but I don\u0027t think the conditional update mechanism actually uses subtransactions; instead it forms a complex compare-and-swap query that either succeeds or doesn\u0027t.\n\nAll I can think of is that the subtransactions\u003dTrue was set in case you wanted to update the volume metadata, image volume metadata, and admin volume metadata from some other function and only commit after all 3 were processed one at a time by this function.  But as far as I can tell, we don\u0027t do that anywhere in the cinder code.  (Also, I\u0027ve never actually used subtransactions, so I may be talking out of my butt here.)\n\nIf we *are* using subtransactions, then we need to redesign this, because the subtransactions flag is deprecated in SQA 1.4 and will probably be removed in 2.0.\n\nhttps://docs.sqlalchemy.org/en/14/orm/session_transaction.html#migrating-from-the-subtransaction-pattern","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b66253c7c04cd96b46cc076e757624230480e736","unresolved":false,"context_lines":[{"line_number":3403,"context_line":"    return result"},{"line_number":3404,"context_line":""},{"line_number":3405,"context_line":""},{"line_number":3406,"context_line":"# TODO: We dropped \u0027subtransactions\u003dTrue\u0027 here. Is that an issue?"},{"line_number":3407,"context_line":"def _volume_x_metadata_update("},{"line_number":3408,"context_line":"    context, volume_id, metadata, delete, model, add\u003dTrue, update\u003dTrue"},{"line_number":3409,"context_line":"):"}],"source_content_type":"text/x-python","patch_set":3,"id":"b5f943c6_8bcb53d8","line":3406,"in_reply_to":"765fe29a_b4a1c807","updated":"2022-04-12 10:31:35.000000000","message":"Okay, we\u0027ll be able to validate this once we land the SQLAlchemy 2.0 fixup series at [1]. You\u0027ll notice in the first patch of that series that SQLAlchemy is warning about the use of autocommitted sessions [2] and the Session.begin.subtransactions flag [3]. If those warnings (or at least the first once) are still an issue after we rebase that series on top of this one, we\u0027ll know we\u0027ve an issue. I\u0027ve posted a WIP patch at [4] to remind me to revisit this later and to unblock this particular patch\n\n[1] https://review.opendev.org/q/topic:sqlalchemy-20+project:openstack/cinder+is:open\n[2] https://review.opendev.org/c/openstack/cinder/+/837163/2/cinder/tests/fixtures.py#221\n[3] https://review.opendev.org/c/openstack/cinder/+/837163/2/cinder/tests/fixtures.py#242\n[4] https://review.opendev.org/c/openstack/cinder/+/837546","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"de75f91068d774ad95a49c4fde64dd9d22a6fa94","unresolved":true,"context_lines":[{"line_number":3403,"context_line":"    return result"},{"line_number":3404,"context_line":""},{"line_number":3405,"context_line":""},{"line_number":3406,"context_line":"# TODO: We dropped \u0027subtransactions\u003dTrue\u0027 here. Is that an issue?"},{"line_number":3407,"context_line":"def _volume_x_metadata_update("},{"line_number":3408,"context_line":"    context, volume_id, metadata, delete, model, add\u003dTrue, update\u003dTrue"},{"line_number":3409,"context_line":"):"}],"source_content_type":"text/x-python","patch_set":3,"id":"765fe29a_b4a1c807","line":3406,"in_reply_to":"8923fca9_0e1d64d6","updated":"2022-04-12 08:18:17.000000000","message":"TL;DR: It should be safe to drop the subtransactions\n\nAs far as I know the only purpose of using the subtransaction pattern in our code was to support both starting a new transaction (if no session was provided to the method) and joining an existing one for methods such as `volume_update` and `volumes_update`.\n\nWe probably should have used a context manager like the SQLA docs recommend instead...\n\n  @contextlib.contextmanager\n  def transaction(session):\n      if not session.in_transaction():\n          with session.begin():\n              yield\n      else:\n          yield\n\nPersonally using a subtransaction doesn\u0027t really make much sense here (though I could be wrong)...  For consistency we really want an all or nothing when updating.","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"38d6b58a28952dc447b4c05345f0e0f42a8f445e","unresolved":true,"context_lines":[{"line_number":3403,"context_line":"    return result"},{"line_number":3404,"context_line":""},{"line_number":3405,"context_line":""},{"line_number":3406,"context_line":"# TODO: We dropped \u0027subtransactions\u003dTrue\u0027 here. Is that an issue?"},{"line_number":3407,"context_line":"def _volume_x_metadata_update("},{"line_number":3408,"context_line":"    context, volume_id, metadata, delete, model, add\u003dTrue, update\u003dTrue"},{"line_number":3409,"context_line":"):"}],"source_content_type":"text/x-python","patch_set":3,"id":"8923fca9_0e1d64d6","line":3406,"in_reply_to":"a63feadc_13d38d4e","updated":"2022-04-11 21:13:03.000000000","message":"I looked at this some more, and the subtransactions\u003dTrue flag was added here:\n\nhttps://review.opendev.org/c/openstack/cinder/+/38658/3/cinder/db/sqlalchemy/api.py#1277\n\nThe entire function has been refactored since then so that instead of doing individual saves in a loop, the function figures out what to do in memory, and does a single bulk_save at the end, which is more efficient and also avoids the need to use subtransactions in the first place.  So I think dropping the flag is OK.  (Still want other opinions, though!)","commit_id":"b8d0df5277eb402b3e56ef0b2ceb7d85d4193b9a"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"28afa9a8780bde6aa323cb9489cd5c3a17412d35","unresolved":true,"context_lines":[{"line_number":3406,"context_line":"    return result"},{"line_number":3407,"context_line":""},{"line_number":3408,"context_line":""},{"line_number":3409,"context_line":"# TODO: We dropped \u0027subtransactions\u003dTrue\u0027 here. Is that an issue?"},{"line_number":3410,"context_line":"def _volume_x_metadata_update("},{"line_number":3411,"context_line":"    context, volume_id, metadata, delete, model, add\u003dTrue, update\u003dTrue"},{"line_number":3412,"context_line":"):"}],"source_content_type":"text/x-python","patch_set":4,"id":"7d5367c4_30e0ccb4","line":3409,"range":{"start_line":3409,"start_character":19,"end_line":3409,"end_character":41},"updated":"2022-05-24 10:50:04.000000000","message":"reading about this from sqlalchemy docs, I don\u0027t feel much worried about this being removed.\n\nWhile the Session still uses the “subtransactions” pattern internally, it is not suitable for end-user use as it leads to confusion, and additionally it may be removed from the Session itself in version 2.0 once “autocommit” mode is removed.","commit_id":"0a4a23514117475e437aea58935f86e89d7f1c7a"}]}