Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22404 )
Change subject: IMPALA-13694: Add ImpalaTestSuite.__reset_impala_clients method ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py@368 PS2, Line 368: super(CustomClusterTestSuite, self).setup_method(method) Why not always call the super setup_method? http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py@402 PS2, Line 402: super(CustomClusterTestSuite, self).teardown_method(method) Why not always call the super teardown_method? http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py File tests/common/test_vector.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@102 PS2, Line 102: option_name = option_name.lower() Nit: unnecessary variable declaration. http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@108 PS2, Line 108: option_name = option_name.lower() Nit: unnecessary variable declaration. http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@113 PS2, Line 113: del self.get_value(EXEC_OPTION_KEY)[option_name] Nit: unnecessary variable declaration. http://gerrit.cloudera.org:8080/#/c/22404/2/tests/hs2/hs2_test_suite.py File tests/hs2/hs2_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/hs2/hs2_test_suite.py@123 PS2, Line 123: # TODO: Rename this method to connect() and provide proper py.test setup_method(). Will these todos be addressed in this change? http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_exprs.py@227 PS2, Line 227: vector.get_value('protocol') There are a lot of instances of this code sprinkled throughout this patch. Would it be possible to expand the default_impala_client and create_impala_client functions to accept a new 'vector' parameter, that if specified, would perform the 'vector.get_value('protocol')'? http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_hdfs_caching.py File tests/query_test/test_hdfs_caching.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_hdfs_caching.py@110 PS2, Line 110: execute_query_using_client Please consider adding an execute_query_using_default_client function which will remove the need to first call self.default_inpala_client. -- To view, visit http://gerrit.cloudera.org:8080/22404 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb47fec9f384cb58b19fdbd10ff7aa0850ad6277 Gerrit-Change-Number: 22404 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 30 Jan 2025 00:42:26 +0000 Gerrit-HasComments: Yes
