[Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend

["Lars Volker (Code Review)" <ge...@cloudera.org>]

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 <ma...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes