Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22362 )
Change subject: IMPALA-13682: Implement missing capabilities in ImpylaHS2Client ...................................................................... Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py@221 PS9, Line 221: > nit: passed Done http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py@298 PS9, Line 298: LOG.info("-- {0}".format(message)) > It could be helpful to quite early if based on the current state the goal s Done http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py@559 PS9, Line 559: return HS2_HTTP > why not include impyla.connect also in the try block? Old code. Moved it inside try. http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py@622 PS9, Line 622: def close(self): > maybe do some logging in this case? Added __log_execute(self, cursor, user). It should be distinctive enough for any case. http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_connection.py@754 PS9, Line 754: self.log_handle(operation_handle, 'getting state') > return early in error state? is_admitted return True if state is ERROR. This is inline with code in ImpalaBeeswaxClient.wait_for_admission_control() https://github.com/apache/impala/blob/f50514a26557b75e476f6a02772cf57d8cfef6fa/tests/beeswax/impala_beeswax.py#L349 http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_test_suite.py@347 PS9, Line 347: sel > shouldn't this be self? Good point. Done. http://gerrit.cloudera.org:8080/#/c/22362/9/tests/common/impala_test_suite.py@351 PS9, Line 351: sel > shouldn't this be self? Good point. Done. http://gerrit.cloudera.org:8080/#/c/22362/9/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/22362/9/tests/custom_cluster/test_admission_controller.py@1897 PS9, Line 1897: IMPALA_CONNECT > Couldn't we simply use Exception or have some wrapper to handle both hs2 an Done http://gerrit.cloudera.org:8080/#/c/22362/9/tests/util/thrift_util.py File tests/util/thrift_util.py: http://gerrit.cloudera.org:8080/#/c/22362/9/tests/util/thrift_util.py@85 PS9, Line 85: query_id > Isn't it session id? I miss this. -- To view, visit http://gerrit.cloudera.org:8080/22362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ac07732424c16338e060c9392100b54337f11b8 Gerrit-Change-Number: 22362 Gerrit-PatchSet: 10 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[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-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 13 Feb 2025 01:58:17 +0000 Gerrit-HasComments: Yes
