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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
