impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
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:

(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 <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message