Gabor Kaszab 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 6: (10 comments) 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 > It seems ok that it doesn't reproduce it every time - it's inherently racy. The test reproducing this seems pretty stable. I'll run the production/ASAN tests, sure. 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: QueryCancelledByShellEx > Can we call this QueryCancelledByShellException or ExpectedQueryCancellatio Sure, I'll go for QueryCancelledByShellException. http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@60 PS5, Line 60: class ImpalaClient(object): > The value might not be necessary if we're always going to pass in the same Done http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@83 PS5, Line 83: > Can you add a brief comment explaining how this works. I.e. that it's set t Done http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@435 PS5, Line 435: if suppress_error_on_cancel and self.is_query_cancelled: > It's weird that we don't need the logic for QueryNotFoundException. Does Im I haven't found any usage in the non-generated part of Impala. I'll add the cancellation check here, though to make consistent. 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: (verb, num_rows, error_report, time_elapsed)) > It might be to just not print anything - we don't always throw this excepti Done 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, ' + \ > Maybe add a few more tables to the cross join to make sure that it runs for Well, this already runs for more than 20s but I see your point, we can be on the safe side adding more tables. With 2 more tables the runtime increases over 5min :) Done http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@342 PS5, Line 342: args = '-q "select * from tpch.customer c1, tpch.customer c2, ' + \ > You could also do something like: I think this query should be fine. I gave it a try and after ~30 mins it still haven't reached the fetching state. http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@346 PS5, Line 346: > Isn't this the command-line arguments? Done http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@349 PS5, Line 349: execution in fact starts and then cancels it. Expects the query > Can you add a comment to the function explaining that it waits 3 seconds be Sure, I'll add these comments to the function description. In fact, this 3 sec is a magic number I took from the other cancellation tests. I played around a little bit on my local machine and started decreasing the value. Apparently, the turning point is somewhere between 0.2 and 0.1 secs where the test starts failing. To be on the safe side, I decided to go for 0.5sec. Do you think it's reasonable? (I'll kick of a release and asan test in Jenkins to see how they behave with this value) -- 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: 6 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 23:51:26 +0000 Gerrit-HasComments: Yes
