Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22358 )
Change subject: IMPALA-13672: Migrate query_test/test_kudu.py to use hs2 protocol ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/22358/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22358/4//COMMIT_MSG@12 PS4, Line 12: - Remove usage of deprecated cursor and unique_cursor fixture. > it could be noted that these used hs2 protocol, which means that some tests Added detailed documentation in default_test_protocol(). I think it is better to explain there. http://gerrit.cloudera.org:8080/#/c/22358/4//COMMIT_MSG@27 PS4, Line 27: This patch added several new methods in ImpalaTestSuite: : - default_test_protocol() to allow individual test class to override its : default test procol. : - execute_query_using_vector() for querying using default client that : match 'protocol' dimension while configuring it with 'exec_option' : dimension. > If we always use hs2 within the test_suite, couldn't we ensure in setup_cla I initially plan to leave it compatible with either protocol. But on second thought, I think it is better to move it permanently so that new test will not accidentally continue using beeswax. This made possible by overriding default_test_protocol() method. http://gerrit.cloudera.org:8080/#/c/22358/4/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22358/4/tests/common/impala_connection.py@566 PS4, Line 566: # ImpalaBeeswaxResult store profile at runtime_profile field : self.runtime_profile = profile : self.query_id = query_id > Can't we use a single member in both? I think only test_observability.py refer to result.profile. I'll start a DRY_RUN to check. http://gerrit.cloudera.org:8080/#/c/22358/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22358/4/tests/common/impala_test_suite.py@340 PS4, Line 340: t > nit: +2 indentation Inlined. http://gerrit.cloudera.org:8080/#/c/22358/4/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/22358/4/tests/query_test/test_kudu.py@173 PS4, Line 173: assert kudu_client.table_exists( > nit: +2 indentation - this files seems quite inconsistent about indentation Replaced this with function call to update vector. We will continue to have inconsistent indentation if we do not have command line tool to auto format such as clang-format or autopep8. Even autopep8 does not fit our coding standard out of the box. For example: https://stackoverflow.com/questions/54430470/vs-code-python-autopep8-does-not-honor-2-spaces-hanging-indentation In patch set 4, I use autopep8 + some hand edit to format test_kudu.py like following: autopep8 --ignore E121 --indent-size 2 --max-line-length 90 -i common/impala_test_suite.py We can install autopep8 to Impala's ./infra/python/deps/py3-requirements.txt. But we'll need to upgrade pylint first so we can get pycodestyle >= 2.10.0. -- To view, visit http://gerrit.cloudera.org:8080/22358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f38baf5a0bbde1a1ad0bb4666c300f4f3cabd33 Gerrit-Change-Number: 22358 Gerrit-PatchSet: 5 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Sun, 26 Jan 2025 05:18:14 +0000 Gerrit-HasComments: Yes
