Tim Armstrong has posted comments on this change. Change subject: IMPALA-4348 / IMPALA-4333: Improve coordinator fragment cancellation ......................................................................
Patch Set 2: (6 comments) Starting to feel a little more comfortable with it. Probably needs some more thought but I did a full pass. http://gerrit.cloudera.org:8080/#/c/4865/2//COMMIT_MSG Commit Message: 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, just some things to wake up the GetNext() thread sooner, right? http://gerrit.cloudera.org:8080/#/c/4865/1/be/src/exec/plan-root-sink.cc File be/src/exec/plan-root-sink.cc: Line 163: > Agree it's not needed for correctness. consumer_cv_wait_ can wake up spurio Yeah, I think we should be clearer what's necessary for correctness and what is an optimisation. http://gerrit.cloudera.org:8080/#/c/4865/2/be/src/exec/plan-root-sink.h 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? http://gerrit.cloudera.org:8080/#/c/4865/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS2, Line 166: rpc_sent = true We don't seem to do anything with this argument. Also, IMO probably better to make it a non-default argument since the default arg adds additional subtlety to something that's already fairly subtle. 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? Would be good to be clear about that. It seems a little strange that it would disappear without leaving an error, but is that possible with cancellation? http://gerrit.cloudera.org:8080/#/c/4865/2/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 507: if (root_sink_ != nullptr) root_sink_->CloseConsumer(); It's a little hard to understand why the producer-side is calling CloseConsumer() without reading the commit message - probably a comment is needed. -- 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: 2 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
