Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4348 / IMPALA-4333: Improve coordinator fragment cancellation ......................................................................
Patch Set 1: (6 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: if (consumer_done_) return state->CheckQueryState(); if someone called CloseConsumer(), does it even make sense to keep executing this function? shouldn't you just return eos at this point? Line 163: consumer_cv_.wait(l); > Agree it's not needed for correctness. consumer_cv_wait_ can wake up spurio make that part of the declared interface (if you call CloseConsumer() then GetNext() will immediately exit with eos). 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 possible error status Line 166: void SetInitialStatus(const Status& status, bool rpc_sent = true) { remove default. Line 506: // If there was an error, but the coordinator fragment was successfully started, it since they're all started at the same time now, why is the coordinator fragment still special? Line 522: executor_ = root_fragment_instance_->executor(); get rid of executor_ and root_sink_ and add executor() and root_sink() that go through root_fragment_instance_, to make clear that those are only valid as long as root_fragment_instance_ is valid. -- 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: 1 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
