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 5: (4 comments) We discussed this issue with Tim from a different angle with the following outcome: - The only way to guarantee that there is no race condition around is_query_cancelled flag is to Introduce locking around that variable. - To make this work well the flag should be checked inside _do_rpc right before the rpc() call. In addition the rpc() call itself should be protected by the same lock block we use for is_query_cancelled check. - Unfortunately, as a result cancelling a query would have to wait until the actual rpc() call is finished. If it runs long then we've shot ourselves in the foot as the error wont be shown at the end but the query cancellation would lag for long. For a solution we considered a symptomatic treatment like approach to let the whole cancellation race condition throw it's exception and then where we catch that exception we can decide to suppress it or not. 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 > So, um, if you want a query that returns a lot of rows: Thanks for the nice example, Phil! I created a TC using your example and it seems to stably reproduce the issue. I wonder what is the probability of this test to become flaky. (I executed it like 100 times and seems to be still stable) http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@319 PS3, Line 319: def wait_to_finish(self, last_query_handle, periodic_callback=None): > Do we also need to check is_cancelled here? In the case of a long-running D Indeed. http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@335 PS3, Line 335: """Fetch all the results. > Why does this need to be set back to False? Seems confusing since the query This in fact not needed here. Done http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@355 PS3, Line 355: if not result.has_more: > I think some of these other methods might also be subject to the same bug - Good point, thx. -- 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: Sat, 18 Nov 2017 00:33:43 +0000 Gerrit-HasComments: Yes
