impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
Date Thu, 08 Sep 2016 04:47:19 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3902: Scheduler improvements for running multiple fragment instances
on a single backend

Patch Set 3:


Just a few comments-on-comments. I'll take another pass over it all soon.
File be/src/runtime/

PS3, Line 708: << query_id_;
> this is accepted indentation
But not preferred. We correct this in code reviews.

PS3, Line 713: CancelOnStartupError
> FinishQueryStartup()?

PS3, Line 1158: (Instances have a sink only if the coordinator does not)
> yes, only insert plans have multiple sinks (and no coord fragment)
Not really - every instance has a sink. Only the coordinator instance may not.

PS3, Line 1446: r
> historically, single-sentence comments were not capitalized and punctuated.
I find it easier to read properly punctuated comments (particularly capitalized ones). Otherwise
they look like sentence fragments (and in one case recently a comment had been chopped in
two; the lack of punctuation meant it wasn't immediately clear).

PS3, Line 1654: if (exec_state->fragment_id() == 0) continue;  // the coord fragment
> we haven't so far.
Not sure what that means - you've added code to explicitly skip the coordinator fragment.
Does that mean it now gets cancelled elsewhere, or it always did and we were redundantly cancelling

PS3, Line 2214: static void DistributeFilters(shared_ptr<TPublishFilterParams> params,
              :     const TNetworkAddress& impalad, const TUniqueId& fragment_instance_id)
> i'll switch it back, but in general please have specific reasons for asking
Sorry, in case I wasn't clear: standardisation is my goal. Having two idioms to achieve the
same goal doesn't make sense, and anonymous namespaces are more general.
File be/src/scheduling/query-schedule.h:

Line 184:   MtFragmentExecParams* GetFragmentExecParams(FragmentId id) {
> it has to be mutable, because it gets created in the scheduler.
What's 'it' - the QuerySchedule? It doesn't get created in the scheduler, but in the QES,
and that's what I'm saying is a problem: it should get created in the scheduler and then returned
as an immutable object to the QES to store for the rest of the query.
File be/src/scheduling/

PS3, Line 483: // that instance gets all of the scan ranges, if there are any
> no. rephrased.
An example of how lack of capitalization doesn't help the reader.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message