Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8549 )
Change subject: IMPALA-1144: Fix exception when cancelling query in Impala-shell with CTRL-C ...................................................................... Patch Set 5: (10 comments) Overall approach seems good to me, but had quite a few comments about comments and naming. http://gerrit.cloudera.org:8080/#/c/8549/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8549/2//COMMIT_MSG@25 PS2, Line 25: Change-Id: I6cefaf1dae78baae238289816a7cb9d210fb38e2 > Thanks for the nice example, Phil! It seems ok that it doesn't reproduce it every time - it's inherently racy. We could probably have a deterministic unit test by mocking out enough things, but I think I prefer the end-to-end shell test. It would be good to also test on release and ASAN builds to make sure that it doesn't immediately fail when the daemon is slow or fast. http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@58 PS5, Line 58: QueryCancelledException Can we call this QueryCancelledByShellException or ExpectedQueryCancellationException or something like that to distinguish from the case when the query was cancelled unexpectedly (e.g. by an admin from the impala debug ui). http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@60 PS5, Line 60: self.value = value The value might not be necessary if we're always going to pass in the same string. The existing exceptions do this but seems like an odd pattern. We could just print that string instead of stringifying the exception. http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@83 PS5, Line 83: self.is_query_cancelled = False Can you add a brief comment explaining how this works. I.e. that it's set to true by the cancellation signal handler and suppresses any errors after cancellation. http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@435 PS5, Line 435: except BeeswaxService.QueryNotFoundException: It's weird that we don't need the logic for QueryNotFoundException. Does Impala even throw this? I'm ok with leaving this in since it's part of the beeswax protocol but we should also check for cancellation for consistency. http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_shell.py@1026 PS5, Line 1026: print_to_stderr(str(e)) It might be to just not print anything - we don't always throw this exception when the query is cancelled so the message only gets printed sometimes. http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@333 PS5, Line 333: 'select * from v, v v2, v v3, v v4, v v5, v v6, v v7, v v8, v v9;"' Maybe add a few more tables to the cross join to make sure that it runs for a very long time. We don't want a flaky test if it sometimes finishes quicker. http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@342 PS5, Line 342: 'tpch.customer c3 order by c1.c_name"' You could also do something like: order by c1.c_name, sleep(1) That would make it pause 1ms per row and slow it down further. http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@346 PS5, Line 346: query_txt Isn't this the command-line arguments? http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@349 PS5, Line 349: sleep(3) Can you add a comment to the function explaining that it waits 3 seconds before cancelling and expects the query to be cancelled. Is there any reason why 3 seconds is the right amount of time? Currently it just seems like a magic number. Would be good to reduce it if possible, since it will make the tests very slow (other shell tests have this problem already). -- To view, visit http://gerrit.cloudera.org:8080/8549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6cefaf1dae78baae238289816a7cb9d210fb38e2 Gerrit-Change-Number: 8549 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 20 Nov 2017 18:41:17 +0000 Gerrit-HasComments: Yes
