impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(8 comments)

Just a few comments-on-comments. I'll take another pass over it all soon.

http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

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
here?


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.


http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/query-schedule.h
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.


http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:

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 http://gerrit.cloudera.org:8080/4054
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message