)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e1a97411e22b73ad1e7b9ad05ad8369d25e5603b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"5079804b_0394e2bd","updated":"2023-02-23 11:14:32.000000000","message":"Comments inline. Also, any chance of a small reproducer if possible?","commit_id":"3d8a63254da3a9a259e2546b86f1474b3a28270c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"33da9fc1198cc4295f9a6ee66aa848140273df89","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"437ed93c_d13b6e30","in_reply_to":"5079804b_0394e2bd","updated":"2023-02-23 14:56:19.000000000","message":"Done","commit_id":"3d8a63254da3a9a259e2546b86f1474b3a28270c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"98ff9a71e2798153fd75527d24fc5fa84b805f2b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"a22413db_043722e6","updated":"2023-02-24 19:18:02.000000000","message":"I would suggest these things:\n\n1. we already have a lot of \"connect to the database\" tests in test_sqlalchemy.py, probably just keep the test there instead of adding an outlier file\n2. the test should be broken up into the \"functional\" one and the \"look at ._transaction\" one\n3. the \"functional\" one should be able to verify behavior just using \"engine.begin()\" success as a test.\n\n","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"50cd53b4ae2561749deadb1007875b9921affaa3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"ad86bd6c_50dcffe0","updated":"2023-03-07 00:26:44.000000000","message":"LGTM, fixes the transaction issue I saw in designate","commit_id":"877bcfc6a6ed16ba6885f47824df6b1f5ac60b4e"},{"author":{"_account_id":22623,"name":"Erik Olof Gunnar Andersson","email":"eandersson@blizzard.com","username":"eoandersson"},"change_message_id":"c51428c9f23a98f928a29f33ecf7b04b1f3acbb5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"03846a4e_983c2fa0","updated":"2023-03-20 15:50:36.000000000","message":"recheck test_server_rescue (and a few other test failures)","commit_id":"877bcfc6a6ed16ba6885f47824df6b1f5ac60b4e"}],"oslo_db/sqlalchemy/engines.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e1a97411e22b73ad1e7b9ad05ad8369d25e5603b","unresolved":true,"context_lines":[{"line_number":84,"context_line":"        connection.scalar(select(1))"},{"line_number":85,"context_line":"    finally:"},{"line_number":86,"context_line":"        connection.should_close_with_result \u003d save_should_close_with_result"},{"line_number":87,"context_line":"        if hasattr(connection, \u0027rollback\u0027):"},{"line_number":88,"context_line":"            connection.rollback()"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"dfb91ce1_7b669033","line":87,"updated":"2023-02-23 11:14:32.000000000","message":"Could you add a comment indicating why we\u0027re doing this? We probably also want a TODO to drop the hasattr call once we bump the minimum to SQLAlchemy 2.0?","commit_id":"3d8a63254da3a9a259e2546b86f1474b3a28270c"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"33da9fc1198cc4295f9a6ee66aa848140273df89","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        connection.scalar(select(1))"},{"line_number":85,"context_line":"    finally:"},{"line_number":86,"context_line":"        connection.should_close_with_result \u003d save_should_close_with_result"},{"line_number":87,"context_line":"        if hasattr(connection, \u0027rollback\u0027):"},{"line_number":88,"context_line":"            connection.rollback()"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1de72abd_e19d6e90","line":87,"in_reply_to":"dfb91ce1_7b669033","updated":"2023-02-23 14:56:19.000000000","message":"Done","commit_id":"3d8a63254da3a9a259e2546b86f1474b3a28270c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"6e57a8d95894b26c21d4915816c3a611b6d26e7e","unresolved":true,"context_lines":[{"line_number":73,"context_line":"        # any details like that needed by the backend are handled."},{"line_number":74,"context_line":"        connection.scalar(select(1))"},{"line_number":75,"context_line":"    except exception.DBConnectionError:"},{"line_number":76,"context_line":"        # catch DBConnectionError, which is raised by the filter"},{"line_number":77,"context_line":"        # system."},{"line_number":78,"context_line":"        # disconnect detected.  The connection is now"},{"line_number":79,"context_line":"        # \"invalid\", but the pool should be ready to return"}],"source_content_type":"text/x-python","patch_set":4,"id":"9c27df3b_ec1d35fb","line":76,"updated":"2023-03-02 12:58:08.000000000","message":"it needs one here too, after the exception catch\n\n    except exception.DBConnectionError:\n        if hasattr(connection, \u0027rollback\u0027):\n            connection.rollback()\n            \nsee https://review.opendev.org/c/openstack/oslo.db/+/875986/3/oslo_db/sqlalchemy/engines.py#147 where I\u0027m trying to get all remaining 2.0 things laid out","commit_id":"349393dd8414736c86ebe683ce4504d34fc6c335"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"2d095d7d6b7c0248ab11e5fe6cd4d236603de854","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        # any details like that needed by the backend are handled."},{"line_number":74,"context_line":"        connection.scalar(select(1))"},{"line_number":75,"context_line":"    except exception.DBConnectionError:"},{"line_number":76,"context_line":"        # catch DBConnectionError, which is raised by the filter"},{"line_number":77,"context_line":"        # system."},{"line_number":78,"context_line":"        # disconnect detected.  The connection is now"},{"line_number":79,"context_line":"        # \"invalid\", but the pool should be ready to return"}],"source_content_type":"text/x-python","patch_set":4,"id":"4e81c0bd_2e710a23","line":76,"in_reply_to":"9c27df3b_ec1d35fb","updated":"2023-03-02 16:52:14.000000000","message":"Thanks for the tip","commit_id":"349393dd8414736c86ebe683ce4504d34fc6c335"}],"oslo_db/tests/sqlalchemy/test_engines.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"42cc20e3e20dad170f99249903070ff63c9c0342","unresolved":false,"context_lines":[{"line_number":37,"context_line":"                self.assertTrue("},{"line_number":38,"context_line":"                    isinstance(conn._transaction, base_engine.RootTransaction))"},{"line_number":39,"context_line":"                engines._connect_ping_listener(conn, False)"},{"line_number":40,"context_line":"                self.assertIsNone(conn._transaction)"}],"source_content_type":"text/x-python","patch_set":2,"id":"cc2a112d_00db9db5","line":40,"range":{"start_line":40,"start_character":0,"end_line":40,"end_character":9},"updated":"2023-02-23 16:54:40.000000000","message":"I made this test with sqlalchemy 2.0, not 1.4. With 1.4, this is not None because connection hasn\u0027t have \"rollback\".","commit_id":"05bcc2b78b1d0f58f2249adc69a3e2296f1fd54c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"98ff9a71e2798153fd75527d24fc5fa84b805f2b","unresolved":true,"context_lines":[{"line_number":24,"context_line":"from oslo_db.tests.sqlalchemy import base as db_test_base"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"class ConnectPingListenerTest(db_test_base._DbTestCase):"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test__connect_ping_listener(self):"},{"line_number":30,"context_line":"        metadata \u003d schema.MetaData()"}],"source_content_type":"text/x-python","patch_set":3,"id":"88f8fc56_c4b4379c","line":27,"updated":"2023-02-24 19:18:02.000000000","message":"This test I think wont actually run against MySQL unless it\u0027s subclassing MySQLOpportunisticTestCase; the ping listener seems to be installed in all cases so I guess using mysql for real is not critical, though it seems like overkill to add a new test file here.  Most of this stuff is in test_sqlalchemy.py right now, not a great name but that\u0027s where all the \"engine connect\" stuff is.","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"30de1371174700b3f23b0619eb319402edddda01","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from oslo_db.tests.sqlalchemy import base as db_test_base"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"class ConnectPingListenerTest(db_test_base._DbTestCase):"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test__connect_ping_listener(self):"},{"line_number":30,"context_line":"        metadata \u003d schema.MetaData()"}],"source_content_type":"text/x-python","patch_set":3,"id":"52f3afe7_df1a7aa4","line":27,"in_reply_to":"88f8fc56_c4b4379c","updated":"2023-03-02 09:05:08.000000000","message":"I\u0027ll move this test to test_sqlalchemy.py","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"98ff9a71e2798153fd75527d24fc5fa84b805f2b","unresolved":true,"context_lines":[{"line_number":33,"context_line":"            schema.Column(\u0027id\u0027, types.Integer, primary_key\u003dTrue),"},{"line_number":34,"context_line":"            mysql_engine\u003d\u0027InnoDB\u0027"},{"line_number":35,"context_line":"        )"},{"line_number":36,"context_line":"        metadata.create_all(self.engine, checkfirst\u003dFalse)"},{"line_number":37,"context_line":"        for idx in range(2):"},{"line_number":38,"context_line":"            with self.engine.begin() as conn:"},{"line_number":39,"context_line":"                self.assertTrue(isinstance(conn._transaction,"}],"source_content_type":"text/x-python","patch_set":3,"id":"89b67bc9_4ae03eb4","line":36,"updated":"2023-02-24 19:18:02.000000000","message":"the Table + create_all() here seems like a different test then the engine.begin() elements below.    It should probably be a separate test.\n\nalso it can be more focused on the issue that occurred from a functional standpoint.   Just doing \"with engine.begin() as conn\" is sufficient.","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"30de1371174700b3f23b0619eb319402edddda01","unresolved":false,"context_lines":[{"line_number":33,"context_line":"            schema.Column(\u0027id\u0027, types.Integer, primary_key\u003dTrue),"},{"line_number":34,"context_line":"            mysql_engine\u003d\u0027InnoDB\u0027"},{"line_number":35,"context_line":"        )"},{"line_number":36,"context_line":"        metadata.create_all(self.engine, checkfirst\u003dFalse)"},{"line_number":37,"context_line":"        for idx in range(2):"},{"line_number":38,"context_line":"            with self.engine.begin() as conn:"},{"line_number":39,"context_line":"                self.assertTrue(isinstance(conn._transaction,"}],"source_content_type":"text/x-python","patch_set":3,"id":"67576270_c527e16d","line":36,"in_reply_to":"89b67bc9_4ae03eb4","updated":"2023-03-02 09:05:08.000000000","message":"Actually the issue seems to be only when calling engine.begin(). I\u0027ll focus the testing only on this.","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"98ff9a71e2798153fd75527d24fc5fa84b805f2b","unresolved":true,"context_lines":[{"line_number":35,"context_line":"        )"},{"line_number":36,"context_line":"        metadata.create_all(self.engine, checkfirst\u003dFalse)"},{"line_number":37,"context_line":"        for idx in range(2):"},{"line_number":38,"context_line":"            with self.engine.begin() as conn:"},{"line_number":39,"context_line":"                self.assertTrue(isinstance(conn._transaction,"},{"line_number":40,"context_line":"                                           base_engine.RootTransaction))"},{"line_number":41,"context_line":"                engines._connect_ping_listener(conn, False)"}],"source_content_type":"text/x-python","patch_set":3,"id":"8696a781_c3d70cb0","line":38,"updated":"2023-02-24 19:18:02.000000000","message":"this part is fine as a separate test, though it\u0027s obviously a little more brittle.  no plans to change \"_transaction\" / RootTransaction right now so should be fine.","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"30de1371174700b3f23b0619eb319402edddda01","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        )"},{"line_number":36,"context_line":"        metadata.create_all(self.engine, checkfirst\u003dFalse)"},{"line_number":37,"context_line":"        for idx in range(2):"},{"line_number":38,"context_line":"            with self.engine.begin() as conn:"},{"line_number":39,"context_line":"                self.assertTrue(isinstance(conn._transaction,"},{"line_number":40,"context_line":"                                           base_engine.RootTransaction))"},{"line_number":41,"context_line":"                engines._connect_ping_listener(conn, False)"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa086d95_f3095859","line":38,"in_reply_to":"8696a781_c3d70cb0","updated":"2023-03-02 09:05:08.000000000","message":"I know I\u0027m accessing a private member and that is usually a source of problems. But I guess that, whenever is changed, this test will fail (and I don\u0027t know if that is good or now, TBH).","commit_id":"e95ea97b75d33bc4869532177152c52e712d267e"}],"oslo_db/tests/sqlalchemy/test_sqlalchemy.py":[{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"5f991f1dd84603ab4f7b0faa5782d6d350f69083","unresolved":false,"context_lines":[{"line_number":909,"context_line":"                engines._connect_ping_listener(conn, False)"},{"line_number":910,"context_line":"                # TODO(ralonsoh): drop this check once SQLAlchemy minimum"},{"line_number":911,"context_line":"                #  version is 2.0."},{"line_number":912,"context_line":"                sqla_version \u003d versionutils.convert_version_to_tuple("},{"line_number":913,"context_line":"                    sqlalchemy.__version__)"},{"line_number":914,"context_line":"                if sqla_version[0] \u003e\u003d 2:"},{"line_number":915,"context_line":"                    self.assertIsNone(conn._transaction)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5e23b90c_774cf91e","line":912,"updated":"2023-03-20 13:23:29.000000000","message":"i think we had tests for this already, but this is fine","commit_id":"877bcfc6a6ed16ba6885f47824df6b1f5ac60b4e"}]}