Henry Robinson has posted comments on this change.

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

Patch Set 12:


This patch includes planner test changes.

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.

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.

File be/src/runtime/coordinator.h:

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

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

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 

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

Reply via email to