Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201609082014.u88KER2J008751@ip-10-146-233-104.ec2.internal>
Date: Thu, 8 Sep 2016 20:14:26 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: Marcel Kornacker <marcel@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Matthew Jacobs <mj@cloudera.com>,
        Alex Behm <alex.behm@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3902=3A_Scheduler_improvements_for_running_multiple_fragment_instances_on_a_single_backend=0A?=
In-Reply-To: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@gerrit.cloudera.org>
References: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 08 Sep 2016 20:14:33 -0000

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