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 3: (4 comments) 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: elif query_state == self.query_state["EXCEPTION"]: Do we also need to check is_cancelled here? In the case of a long-running DML query like an insert, not sure if an error can bubble up here. http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@334 PS3, Line 334: if self.is_query_cancelled: How do we avoid the two threads racing? E.g. * Fetch thread checks is_query_cancelled, sees False, then starts fetch RPC * Cancellation thread sets is_query_cancelled to True and sends Cancelled RPC around the same time * Fetch fails, either with CANCELLED or invalid query handle * The cancelled or invalid query handle bubbles up to the client. Assignment of variables is atomic in Python, so I think it's sufficient to re-check is_query_cancelled after the fetch RPC returns, but we should add some concise comments to explain how concurrent fetch and cancel work. http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@335 PS3, Line 335: self.is_query_cancelled = False Why does this need to be set back to False? Seems confusing since the query is still cancelled and we haven't started a new one at this point. Isn't setting it in execute_query() sufficient. http://gerrit.cloudera.org:8080/#/c/8549/3/shell/impala_client.py@355 PS3, Line 355: def close_dml(self, last_query_handle): I think some of these other methods might also be subject to the same bug - it seems like these RPCs could race with cancellation too. -- 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: 3 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@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: Wed, 15 Nov 2017 17:45:28 +0000 Gerrit-HasComments: Yes
