Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8713 )
Change subject: IMPALA-6265 Query cancellation test enhancements ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@349 PS2, Line 349: def wait_query_until_desired_state(self, stmt, state, max_retry=10): Thanks, this looks good. The name is a little non-idiomatic. Maybe something like wait_for_query_state()? http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@362 PS2, Line 362: return "The found in flight query is not the one under test: " + \ Maybe just raise an exception? Makes it hard for tests to accidentally forget to check the return value http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@366 PS2, Line 366: ++retry_count I don't think ++ works in python. It is syntactically valid but doesn't increment the variable. I think "retry_count += 1" is the most concise way of doing this. http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@368 PS2, Line 368: return "Query didn't reach desired state: " + state Same here - maybe raise an exception or just assert? http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@385 PS2, Line 385: get_statement_from_args It looks like we have the original statement in the tests so it seems better to pass the statement through wait_query_until_desired_state() instead of trying to extract it from the command line. http://gerrit.cloudera.org:8080/#/c/8713/2/tests/shell/test_shell_commandline.py@386 PS2, Line 386: ImapalShell ImpalaShell -- To view, visit http://gerrit.cloudera.org:8080/8713 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0bff485a872df7be8efd784314a6ca91aaadd11 Gerrit-Change-Number: 8713 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 04 Dec 2017 17:39:31 +0000 Gerrit-HasComments: Yes
