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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
