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: (19 comments) http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS3, Line 466: request.query_ctx.request.query_options.mt_dop != 1 parentheses? Sometimes we have them in these cases. http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: Line 562: const TPlanFragment* GetCoordFragment(const TQueryExecRequest& request) const; static? PS3, Line 565: GetTPlanFragments Could this be static? Implementation doesn't seem to access instance members. Line 569: int GetNumRemoteInstances(const QuerySchedule& schedule) const; static? http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: PS3, Line 321: auto use auto only for iterators http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS3, Line 73: sender_id Initialize this? PS3, Line 281: void InitMtFragmentExecParams(); comment? http://gerrit.cloudera.org:8080/#/c/4054/3/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: PS3, Line 384: auto auto only for iterators PS3, Line 403: backend_config nit: some of these parameters could go to the previous line, maybe saving a line. PS3, Line 429: i + 1 Can you add a comment to explain the "+ 1"? Line 434: || sink.output_partition.type == TPartitionType::HASH_PARTITIONED nit: formatting Line 448: src_params->destinations.emplace_back(); You could resize src_params->destinations and then call setters on the elements. The code would be more compact and might be easier to read. PS3, Line 478: MakeNetworkAddress Replace with local_backend_descriptor_.address? Line 509: /// - Incomplete comment? PS3, Line 522: nit: extra space Line 530: // average number of bytes to each instance formatting PS3, Line 534: num_instances can this be 0? PS3, Line 535: int64 we often use int64_t for those types. Is there a difference? PS3, Line 543: static_cast<int64>(avg_bytes_per_instance * (i + 1)) Maybe use a variable for this to make it more readable? -- 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
