Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201605120540.u4C5eWPs015216@ip-10-146-233-104.ec2.internal>
Date: Thu, 12 May 2016 05:40:32 +0000
From: "Marcel Kornacker (Code Review)" <gerrit@cloudera.org>
To: Lars Volker <lv@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Mostafa Mokhtar <mmokhtar@cloudera.com>, anujphadke <aphadke@cloudera.com>
Reply-To: marcel@cloudera.com
Subject: =?UTF-8?Q?[Impala-CR](cdh5-trunk)_IMPALA-2979:_Fix_scheduling_on_remote_hosts=0A?=
In-Reply-To: <gerrit.1455696443000.I044f83806fcde820fcb38047cf6b8e780d803858@gerrit.cloudera.org>
References: <gerrit.1455696443000.I044f83806fcde820fcb38047cf6b8e780d803858@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.10-rc0
archived-at: Thu, 12 May 2016 05:40:38 -0000

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2979: Fix scheduling on remote hosts
......................................................................


Patch Set 22:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/2200/22/be/src/scheduling/simple-scheduler.h
File be/src/scheduling/simple-scheduler.h:

Line 123:   typedef boost::heap::binomial_heap<BackendAssignmentInfo,
does this really require anything from boost (and not std)? we're trying to get rid of boost dependencies.


Line 210:   /// data from the given data_loation.
what is data_location?

does this do load balancing with node_assignments?


Line 216:   /// node_assignments:     Used to track per-query and per-host assignment information.
which backends does this track? is this an output param? the global state?


Line 254:   Status ComputeScanRangeAssignmentForQuery(const TQueryExecRequest& exec_request,
isn't the 'for query' implied?


Line 262:   /// First we assemble a list of candidate backends, from which the scan range can be
no ,


Line 263:   /// read. Backends are classified by their memory distance to the data, and the closest
briefly explain memory distance, it's not a standard term


Line 266:   /// if they were further away. Only backends running on one of the replicas of the scan
i don't think you should describe the steps of the algorithm here (you could read the code for that, plus it might change anyway). instead, describe the outcome/semantic properties of the computed assignment (and be specific there; for instance, list the query option that influence replica preference).


Line 273:   /// list, resulting in a local read. First a search is performed to find the candidate
yep, this is way too detailed.


Line 307:   Status ComputeScanRangeAssignmentForPlanNode(PlanNodeId node_id,
same here, the parameters differentiate it from 'for query'.


Line 350:   /// Lookup the IP address of 'hostname' and return whether the lookup was successful. If
lookup is a noun, to look up is the verb.


Line 358:   Status HostnameToIpAddr(const Hostname& hostname, IpAddr* ip);
on the face of it this looks like LookUpBackendIp. how is it different?


Line 376:       bool global_backend_order, AssignmentInfo* context);
is this the global assignment info?


-- 
To view, visit http://gerrit.cloudera.org:8080/2200
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I044f83806fcde820fcb38047cf6b8e780d803858
Gerrit-PatchSet: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes