)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"73e97c483c499e3fa4ada13b5f7f4efe39feab3e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f4141cee_6d67b91a","updated":"2022-04-14 15:19:10.000000000","message":"Looks correct to me.","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a484fff0b4db166d9b6b12a976a595b8760a114e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e1373b09_759e19fb","updated":"2022-06-07 10:41:43.000000000","message":"Thanks Stephen. LGTM.","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"}],"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"73e97c483c499e3fa4ada13b5f7f4efe39feab3e","unresolved":true,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"88de3291_192c0960","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"updated":"2022-04-14 15:19:10.000000000","message":"Note to reviewers: the only docs I could find for this (which is basically like the sqlalchemy begin_nested()) are the oslo.db unit tests:\n\nhttps://opendev.org/openstack/oslo.db/src/commit/95e1dd6e82e88b123a6118bf421464495d16f290/oslo_db/tests/sqlalchemy/test_enginefacade.py#L1892","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b66215b9e749775a6b5733d7dbd1a1db73bda13c","unresolved":true,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"eb507ba8_cd4e8ca3","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"in_reply_to":"611077b5_c9d70d88","updated":"2022-06-07 09:31:40.000000000","message":"If we did \u0027context.session.add(data)\u0027 then the actual session commit will happen when you exit the function. That means the try-except here is useless - the \u0027data.save()\u0027 call won\u0027t raise \u0027DBDuplicateEntry\u0027, only \u0027context.session.commit()\u0027 will. By using the context manager, the commit happens when we leave the \"with\" block so any exceptions will be raised immediately.\n\nWith that said, we don\u0027t need the decorator. I should drop that. Will post a follow-up shortly.","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"1ed7f5558995b6ff2bc56f0f1ebf4f7024a9b101","unresolved":true,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"611077b5_c9d70d88","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"in_reply_to":"88de3291_192c0960","updated":"2022-06-02 11:46:14.000000000","message":"Can we have some more info regarding the usage of this and why context.session.add(data) is not correct here?","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"56e09efffda967e87e3db3943a65be57212daa96","unresolved":false,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"7881404d_44d19fc3","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"in_reply_to":"c90517b0_212f8084","updated":"2022-06-07 12:24:00.000000000","message":"\u003e \u003e With that said, we don\u0027t need the decorator. I should drop that. Will post a follow-up shortly.\n\u003e \n\u003e This is still true.\n\nThat\u0027s at [1]. I also posted a follow-up to the follow-up [2] that will remove the need for this entirely.\n\n[1] https://review.opendev.org/c/openstack/cinder/+/844961\n[2] https://review.opendev.org/c/openstack/cinder/+/844962","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"87deefc49b695008c4d51636dec0566b19633bfa","unresolved":true,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c90517b0_212f8084","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"in_reply_to":"eb507ba8_cd4e8ca3","updated":"2022-06-07 12:21:31.000000000","message":"\u003e If we did \u0027context.session.add(data)\u0027 then the actual session commit will happen when you exit the function. That means the try-except here is useless - the \u0027data.save()\u0027 call won\u0027t raise \u0027DBDuplicateEntry\u0027, only \u0027context.session.commit()\u0027 will. By using the context manager, the commit happens when we leave the \"with\" block so any exceptions will be raised immediately.\n\nNope, I got this wrong. It\u0027s not because of when the commit happens. Rather, it\u0027s because we\u0027re not handling the exception (we return \u0027True\u0027 rather than raising a different exception). That means we try to commit the change [1] instead of rolling back the transaction [2]. You can prove this by removing this line. I\u0027ll add a note here.\n\n[1] https://github.com/openstack/oslo.db/blob/3f889d5b6e/oslo_db/sqlalchemy/enginefacade.py#L659\n[2] https://github.com/openstack/oslo.db/blob/3f889d5b6e/oslo_db/sqlalchemy/enginefacade.py#L660-L662\n\n\u003e With that said, we don\u0027t need the decorator. I should drop that. Will post a follow-up shortly.\n\nThis is still true.","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a484fff0b4db166d9b6b12a976a595b8760a114e","unresolved":false,"context_lines":[{"line_number":8387,"context_line":"    data.key \u003d key"},{"line_number":8388,"context_line":"    data.value \u003d value"},{"line_number":8389,"context_line":"    try:"},{"line_number":8390,"context_line":"        with main_context_manager.writer.savepoint.using(context):"},{"line_number":8391,"context_line":"            data.save(context.session)"},{"line_number":8392,"context_line":"            return True"},{"line_number":8393,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"aced0752_82c7ad9e","line":8390,"range":{"start_line":8390,"start_character":13,"end_line":8390,"end_character":65},"in_reply_to":"eb507ba8_cd4e8ca3","updated":"2022-06-07 10:41:43.000000000","message":"Ack, thanks for the info.\nIf you plan to do a followup, please add a comment explaining this before the \u0027with\u0027 block so people are aware why we are doing it like this instead of the standard way of decorating the method.","commit_id":"43c1f3107ed99732cecdb7a448edee16749722c9"}]}