Henry Robinson has posted comments on this change. Change subject: IMPALA-4348 / IMPALA-4333: Improve coordinator fragment cancellation ......................................................................
Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/4865/1/be/src/exec/plan-root-sink.cc File be/src/exec/plan-root-sink.cc: 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. http://gerrit.cloudera.org:8080/#/c/4865/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 1550 > leave a comment in place explaining why the cancel call is done despite a p Done Line 166: /// CancelFragmentInstances() will include this instance in the set of potential > remove default. Done 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 http://gerrit.cloudera.org:8080/4865 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b4e25c1d658b3929182ba5e56b5c5e881dd394a Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
