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:58:08 GMT
Henry Robinson has posted comments on this change.

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

Patch Set 5:

File be/src/exec/

Line 156:   results_ = results;
> if someone called CloseConsumer(), does it even make sense to keep executin
We decided not to include this path for this patch - reason being that we want to keep the
number of ways to exit this method small, to make sure we're getting good test coverage. Returning
here has no correctness benefit.
File be/src/runtime/

Line 1550
> leave a comment in place explaining why the cancel call is done despite a p

Line 166:   /// CancelFragmentInstances() will include this instance in the set of potential
> remove default.

Line 506:   }
> since they're all started at the same time now, why is the coordinator frag
The coordinator is the only one we need to worry about closing, so we don't need to do it
ourselves. Updated comment.

Line 522:       FragmentInstanceState* root_state = fragment_instance_states_[0];
> get rid of executor_ and root_sink_ and add executor() and root_sink() that
Will do in follow-on patch - that has a couple of subtle effects that I don't want to worry
about for this fix.

To view, visit
To unsubscribe, visit

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

View raw message