Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all 

Patch Set 12:

File be/src/runtime/

Line 1151:   query_profile_->AddInfoString(
is that not true anymore?
File be/src/runtime/

Line 517:     // After WaitForPrepare() returns, the executor's root sink may 
be set up. At that
why just may be?

Line 1522:   // should explicitly pass Status::OK()). See IMPALA-4279.
that seems counterintuitive, calling cancel and the query then ending up in an 
OK state

Line 2106:   {
does this scope really make a difference? i wouldn't expect much lock 
contention in tear-down. if there's something subtle going on, please leave a 
File be/src/runtime/coordinator.h:

Line 296:   /// FragmentExecState.
and set in Exec()
File be/src/runtime/

Line 231:     // tokens reserved for no reason.
i'm not sure i necessarily buy that (this thread might end up executing merge 
logic, etc.), but let's not worry about that, that'll get fixed when we 
implement thread scheduling

Line 340:       ADD_TIMER(timings_profile_, "ExecTreeExecTime");
how about just ExecTime? (what else is there to execute?)
File be/src/runtime/plan-fragment-executor.h:

Line 112:   /// exec node tree. report_status_cb will have been called for the 
final time when
> The fragment instance, to me, describes the whole thing - sink, exec node t
either that or maybe even exec tree
File be/src/service/

Line 68:   AsciiQueryResultSet(const TResultSetMetadata& metadata)
nice to get rid of all those includes. we should do that more often.
File be/src/service/

Line 321: class HS2RowOrientedResultSet : public QueryResultSet {
why not move all of the qrs subclasses into the qrs header (and the 
implementations into a new .cc) as well? would make it easier to comprehend the 
combined functionality. plus, the various * are already 
large/rambling enough.

Line 476: namespace {
'static' will save you 3 lines :)
File be/src/service/

Line 446:     // only have a single fragment, and that fragment needs to be 
executed by the
revise comment, the coordinator doesn't execute instances anymore

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

Reply via email to