Lars Volker has posted comments on this change. Change subject: IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend ......................................................................
Patch Set 3: (23 comments) http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS4, Line 1377: . nit trailing whitespace PS4, Line 1802: nit: remove http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: PS4, Line 297: ng o nit: Wait() http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: Line 330 For consistency you could change this to use return and a ternary expression, or change the next method to use an if-clause and ++result. Line 343 Using return and a ternary expression might be more concise. PS4, Line 346: nullptr Line 355 You could reserve additional space in fragments before this line to reduce the number of re-allocations. Line 360 reserve() before the loop? http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS3, Line 281: void InitMtFragmentExecParams(); > comment? ? http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: Line 97: std::vector<FInstanceExecParams> instance_exec_params; any benefit from using map over unordered_map here? PS4, Line 173: nullptr Line 174: std::vector<FragmentExecParams>* exec_params() { return &fragment_exec_params_; } nit: trailing whitespace Line 213: RuntimeProfile* summary_profile() { return summary_profile_; } If you need non-const access, isn't it more idiomatic to return a non-const ref, especially since there's also the getter above, returning a const-ref? PS4, Line 219: nullptr http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: PS4, Line 384: to& auto only for iterators PS4, Line 414: isn't this an implementation detail of the other MtComputeFragmentExecParams() method? PS4, Line 470: plan_exec_info, schedul Can we be sure that recursion depth will not be a problem here? Line 524: // TODO: implement logic for hbase and kudu nit: wrap at 90 characters Line 695: // assign instance ids you could call reserve() before this line Line 1104: for (int i = 0; i < backends.size(); ++i) { BackendConfig has been moved to an own file in asf/master, and this code has been removed. Might result in a merge conflict. http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: PS4, Line 489: nit: whitespace, wrap at 90 characters PS4, Line 492: MtFragmentExecParams* fr Is there a way around referring to a private member of the schedule here? PS4, Line 502: Is this implementation-detail relevant for the caller? -- 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
