impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2905: Handle coordinator fragment lifecycle like all others
Date Thu, 13 Oct 2016 05:54:21 GMT
Henry Robinson has posted comments on this change.

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

Patch Set 12:


This patch includes planner test changes.
File be/src/runtime/

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/

Line 517:     // After WaitForPrepare() returns, the executor's root sink may be set up. At
> 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/

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/

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.
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
On further inspection, this isn't necessary. The descriptor table is always set (and always
read by the coordinator).

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