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 Wed, 30 Nov 2016 05:27:11 GMT
Henry Robinson has posted comments on this change.

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


Patch Set 4: Code-Review+1

(7 comments)

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

PS4, Line 294: if (!status.ok()) {
             :    
nit: one line


PS4, Line 298: opened_promise_.Get()
here it seems clearer to return status.


PS4, Line 331: RETURN_IF_ERROR(opened_promise_.Get());
I'm still of the opinion that this logic is better placed in FragmentExecState, e.g.:

  if (fragment->Open().OK()) {
    fragment->Exec();
  }
  fragment->Close();

and then here you could just DCHECK(opened_promise.IsSet() && opened_promise_.Get().OK());
which is a simpler lifecycle invariant. That's in keeping with how the PFE lifecycle is managed
now, and it seems to make sense to have all that logic in one place.

For example, we don't have the same logic in Open() wrt prepare_promise_, which we should
do for consistency with this method as implemented here.


PS4, Line 447: that
the


Line 489: 
how about DCHECK(!report_thread_active_) here? I'd like to get some guarantee that FragmentComplete()
was called in all executions it should have been before Close().


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
> Isn't that what the next sentence is saying? Or am I misunderstanding your 
Oh yeah. I was thrown off by 'disables periodic reporting altogether', which while correct
was confusing.


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

PS4, Line 249:  
Nit: extra space.


-- 
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: 4
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