impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4402/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 1151:   query_profile_->AddInfoString(
is that not true anymore?


http://gerrit.cloudera.org:8080/#/c/4402/12/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

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


http://gerrit.cloudera.org:8080/#/c/4402/12/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 296:   /// FragmentExecState.
and set in Exec()


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

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?)


http://gerrit.cloudera.org:8080/#/c/4402/10/be/src/runtime/plan-fragment-executor.h
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


http://gerrit.cloudera.org:8080/#/c/4402/10/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

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


http://gerrit.cloudera.org:8080/#/c/4402/12/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

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 *-server.cc are already large/rambling enough.


Line 476: namespace {
'static' will save you 3 lines :)


http://gerrit.cloudera.org:8080/#/c/4402/12/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

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 http://gerrit.cloudera.org:8080/4402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb0064ec2f085fa3a5598ea80894fb489a01e4df
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message