Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24026 )
Change subject: IMPALA-14776 (part 1): Use context managers for manually created clients ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/24026/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24026/2//COMMIT_MSG@8 PS2, Line 8: Not sure if you are aware of my patch https://gerrit.cloudera.org/#/c/23992/ which also closes connections in some cases. The two patches seem to touch different places, don't know if this was intentional. http://gerrit.cloudera.org:8080/#/c/24026/2//COMMIT_MSG@20 PS2, Line 20: This modifies most locations using these APIs to use with-as or : try-finally blocks to guarantee cleanup. I was also thinking about how to handle these, and adding a "with" everywhere seemed too noisy. Can't we collect the clients created by these functions in a list, and clean them up at the end of tests? The tests that already close these connections could be handled by adding an option "closed_by_caller=False" arg or by ensuring that double close is NOOP. I am ok with the current solution if you prefer it, but generally I think that it is better if tests have less avoidable init/cleanup code. The "classic" pytest way would be to use fixtures to make the infrastructure needed by a test explicit, e.g "client" / "client_from_vector" / "clients_per_coord". Another thing is that some of the tests changed don't really need to create a client IMO. Added a few comments about this in tests as potential things to clean up. http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_external_planner.py File tests/custom_cluster/test_external_planner.py: http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_external_planner.py@40 PS2, Line 40: with self.create_impala_client() as setup_client: could use execute_query() http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_logging.py File tests/custom_cluster/test_logging.py: http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_logging.py@37 PS2, Line 37: with self.create_impala_client() as client: : self.execute_query_expect_success(client, query, {'max_errors': max_errors}) could use execute_query() http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_re2_max_mem.py File tests/custom_cluster/test_re2_max_mem.py: http://gerrit.cloudera.org:8080/#/c/24026/2/tests/custom_cluster/test_re2_max_mem.py@47 PS2, Line 47: self.execute_query_expect_failure(client, query) : else: : self.execute_query_expect_success(client, query) could use self.client http://gerrit.cloudera.org:8080/#/c/24026/2/tests/metadata/test_ddl_base.py File tests/metadata/test_ddl_base.py: http://gerrit.cloudera.org:8080/#/c/24026/2/tests/metadata/test_ddl_base.py@62 PS2, Line 62: execute could use self.client as pass sync_ddl as query option note that I checked the usage of this function, and it is only used as self._create_db(unique_database) in tests that drop the unique database, which seems awkward, and sync/comment are not used http://gerrit.cloudera.org:8080/#/c/24026/2/tests/metadata/test_event_processing.py File tests/metadata/test_event_processing.py: http://gerrit.cloudera.org:8080/#/c/24026/2/tests/metadata/test_event_processing.py@352 PS2, Line 352: with self.create_impala_client_from_vector(vector) as client: The only reason for creating a client seems to be to change some query options defined in vector - execute_query_using_vector could be used instead or execute_query(..., vector.get_value('exec_option')) -- To view, visit http://gerrit.cloudera.org:8080/24026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib550527838a81cd2aaf69bb715080f6ac6da3786 Gerrit-Change-Number: 24026 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Tue, 24 Feb 2026 12:46:56 +0000 Gerrit-HasComments: Yes
