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 Tue, 29 Nov 2016 20:08:04 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5250/3/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS3, Line 295: FragmentComplete(open_status_);
> Note that Close() always gets called. Do you think it would be simpler to m
I thought about that but purposely wanted to avoid making RPCs in Close(), because when Sailesh
was fixing those datastream sender failure cases, we decided to avoid the pattern of putting
RPCs in Close because there is nowhere to bubble status up to. i.e. Close() would be about
only local teardown, not interprocess teardown which added FlushFinal() for).

Ultimately, I think we should consider moving all of this logic into FES (or it's predecessor)
so that (a) it all happens in one place and (b) to eliminate the need for the callback indirection.


PS3, Line 315: report_thread_started_cv_
> Not your bug, but this is prone to spurious wakeups. Should be:
Done


PS3, Line 330: RETURN_IF_ERROR
> But also, now I think of it - FragmentExecState::Exec() checks to see if Pr
Yeah, that's what I meant in the commit message about cleaning up the interactions. I'll go
one step further to see if it seems better.


-- 
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: 3
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-HasComments: Yes

Mime
View raw message