Henry Robinson has posted comments on this change. Change subject: IMPALA-2905: Handle coordinator fragment lifecycle like all others ......................................................................
Patch Set 12: (10 comments) This patch includes planner test changes. 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? Good catch - got removed when I was investigating where WaitForAllInstances() should have been called. 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? If prepare() fails, the root sink may not be set up. Line 1522: // should explicitly pass Status::OK()). See IMPALA-4279. > that seems counterintuitive, calling cancel and the query then ending up in I think it's ok the cancel fragments without the query being in error. The query may complete successfully, but it still makes sense to cancel the remote fragments that might still be doing work (e.g. queries with a limit). But I don't think the query status should go to cancelled - just the status of the individual fragments. We have a confusing situation where the QES has a query status, but so does the coordinator. We should probably merge them to make it more clear what the source of truth is. This all needs straightening out in a future patch. Line 2106: { > does this scope really make a difference? i wouldn't expect much lock conte It's for readability more than anything - the scope indicates that the lock is not important to hold during CloseConsumer(). That makes it easier to change the logic in the future because there's obviously no dependency between the lock and the other teardown code. 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() Done 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 mer Agreed - this is just inherited from the coordinator. Line 340: ADD_TIMER(timings_profile_, "ExecTreeExecTime"); > how about just ExecTime? (what else is there to execute?) The time driving the sink isn't included (because right now that depends a lot on the client's response time, so doesn't make so much sense to include in the tree exec time). 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 impleme I think I'll do that separately, if you agree. Although it's mostly mechanical, it adds a bunch of noise to this patch. Line 476: namespace { > 'static' will save you 3 lines :) We already talked about this :) IMO, it's better to have a single idiom for anonymising methods, and namespace {} is (incrementally) a little better than static because you can do anonymised typenames as well. Personally I find namespace { } more intuitive as well - static is so overloaded I have to look almost every time to see what it means in which context. 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 On further inspection, this isn't necessary. The descriptor table is always set (and always read by the coordinator). -- 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 <he...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
