impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4348 / IMPALA-4333: Improve coordinator fragment cancellation
Date Fri, 28 Oct 2016 00:13:08 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4348 / IMPALA-4333: Improve coordinator fragment cancellation

Patch Set 2:

Commit Message:

Line 13: access a destroyed PlanRootSink object and crash (IMPALA-4333).
> did this bug reproduce in any tests, or was it only caught by Hue?
Only caught by Hue. Needs some kind of... fault injection framework? That sounds like an idea.

I tested it manually.

Line 25: * Ensure that it is safe to call CloseConsumer() concurrently to
> If I understand correctly there weren't any correctness fixes in this part,
File be/src/exec/

Line 163: 
> Yeah, I think we should be clearer what's necessary for correctness and wha
Now that CloseConsumer() signals consumer_cv_ (which I think is an appropriate functional
change), I think this is relevant to the patch (otherwise I agree, we should remove it).
File be/src/exec/plan-root-sink.h:

Line 79:   /// that calls Send(). *eos is set to 'true' when there are no more rows to consume.
> When does it block/unblock?
File be/src/runtime/

PS2, Line 166:  rpc_sent = true
> We don't seem to do anything with this argument.
Woops, thanks. It's ok for rpc_sent_ to be incorrectly set to true - it's not an error to
try to cancel non-existant plan fragments - but it does prevent warnings in the logs.

PS2, Line 517: might have been cancelled
> what does it mean to "have been cancelled"?  Since line 478 gets the lock, 
Failed is probably better, yep.

Line 523:       // Try and return the fragment instance status if it was already set.
> Are there any situations where the fragment failed and this isn't set? Woul
Reporting the failure happens asynchronously - I think it's very unlikely to be an issue,
but there's nothing guaranteeing that the error is visible to this thread even though the
fragment has finished.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5b4e25c1d658b3929182ba5e56b5c5e881dd394a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message