impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others
Date Wed, 12 Oct 2016 11:22:17 GMT
Marcel Kornacker has posted comments on this change.

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

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
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 comment.
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
> 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

View raw message