impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4504: fix races in PlanFragmentExecutor regarding status reporting
Date Wed, 30 Nov 2016 19:23:18 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4504: fix races in PlanFragmentExecutor regarding status reporting
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5250/7/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS7, Line 51: prepare_promise_
> Can't we just use the PFEs prepared_promise_ ? They both track the same val
Yes, if we expose a timeout interface from PFE. Given this is preexisting and these classes
will probably need major refactoring for MT soon, I'd rather not muck with it now.


PS7, Line 62: DCHECK(status.ok() || done);  // if !status.ok() => done
> Redundant with SendReport()
Yes, but it is a precondition to both, and since this is the dcheck that caught the bug in
the first place, I'm inclined to leave it.  (there's no reason there can't be other callers
to this method, and SendReport() doesn't always call this-- well it does after the coordinator
fragment change, but the code hasn't been fully cleaned up to reflect that yet).


-- 
To view, visit http://gerrit.cloudera.org:8080/5250
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I052b7b4fabb341ad27ad294cd5b0a53728d87d0e
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message