impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Kaszab (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1144: Fix exception when cancelling query in Impala-shell with CTRL-C
Date Sat, 18 Nov 2017 00:33:43 GMT
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 <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Nov 2017 00:33:43 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message