impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
Date Thu, 08 Sep 2016 20:14:26 GMT
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:

File be/src/runtime/

PS4, Line 1377: .
nit trailing whitespace

PS4, Line 1802: 
nit: remove
File be/src/runtime/coordinator.h:

PS4, Line 297: ng o
nit: Wait()
File be/src/scheduling/

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: 

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?
File be/src/scheduling/query-schedule.h:

PS3, Line 281:   void InitMtFragmentExecParams();
> comment?
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: 

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: 
File be/src/scheduling/

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message