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-4504: fix races in PlanFragmentExecutor regarding status reporting
Date Tue, 29 Nov 2016 19:42:57 GMT
Henry Robinson has posted comments on this change.

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


Patch Set 3:

(5 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 move all calls to
FragmentComplete() there?

That would be useful also to remove the explicit call to ReportStatusCb() in FragmentExecState::Prepare(),
but a bit trickier to track what the final status was that you need to report.


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

  while (!report_thread_active_) report_thread_started_cv_.wait(l);


PS3, Line 330: RETURN_IF_ERROR
But also, now I think of it - FragmentExecState::Exec() checks to see if Prepare() was OK().
I think it would be clearest to have it do the same with Open(), and not call Exec() in the
same way.


PS3, Line 330: open_status_
the status is already stored in the promise, so no need to make an extra copy. 

  DCHECK(opened_promise_.IsSet());
  RETURN_IF_ERROR(opened_promise_.Get());


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

PS3, Line 66: setting that flag to 0 disables periodic reporting altogether
Update comment to say that at least one report (the final one) will always be sent.


-- 
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: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message