impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4865/2//COMMIT_MSG
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,
Done


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: 
> 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).


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?
Done


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.
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 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 <henry@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message