Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12530 )
Change subject: IMPALA-8158: Retrieve thrift profiles through Impyla 0.15.0 ...................................................................... Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/12530/11/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/12530/11/tests/common/impala_connection.py@314 PS11, Line 314: "FINISHED_STATE" > do we have a constant for this? like in TCLIService.TOperationState TCLIService.TOperationState is an enum and we need to use _VALUES_TO_NAMES to map it to a string. This makes this whole check: return cursor.status() == TOperationState._VALUES_TO_NAMES[TOperationState.FINISHED_STATE] Since this is thrift generated, the enum value and its string will always be the same and using the constant+map will not give us any more flexibility in the future, but is much less readable. Let me know if you feel strongly about it or if I'm missing anything, but otherwise I think I prefer the string. http://gerrit.cloudera.org:8080/#/c/12530/11/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/12530/11/tests/query_test/test_cancellation.py@221 PS11, Line 221: assert any(client.get_state(handle) == 'RUNNING_STATE' or sleep(1) : for _ in range(5)), 'Query failed to start' > we can use wait_for_state() here and at Line 228 ImpalaTestSuite.wait_for_state uses self.client, which is a beeswax client. Here we want to use the hs2_client though, and I thought that this was easier than adding another set of methods to the test suite. Let me know if you prefer adding a new method wait_for_state_using_client() there. Once we switch our tests to HS2, we can update the usage here. -- To view, visit http://gerrit.cloudera.org:8080/12530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I769a99f0843297dd2b20f2f5b1a9046c97bb131e Gerrit-Change-Number: 12530 Gerrit-PatchSet: 11 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Apr 2019 18:08:51 +0000 Gerrit-HasComments: Yes
