Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708291829.v7TITD5M022728@ip-10-146-233-104.ec2.internal>
Date: Tue, 29 Aug 2017 18:29:13 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Dan Hecht <dhecht@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>
Reply-To: kwho@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4856=3A_Include_KRPC_services_in_plan_fragment=27s_destinations=0A?=
In-Reply-To: <gerrit.1503345128000.I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72@gerrit.cloudera.org>
References: <gerrit.1503345128000.I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72@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.7
archived-at: Tue, 29 Aug 2017 18:29:20 -0000

Michael Ho has posted comments on this change.

Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations
......................................................................


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

PS3, Line 39: Kudu RPC
> that seems confusing since it has nothing to do with Kudu from the users' p
Done


http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h
File be/src/scheduling/backend-config.h:

PS3, Line 60: hostname
> if it's a host name, why is the type TNetworkAddress as opposed to Hostname
This is needed to compare the port too. Apparently, each hostname maps to a list of backend descriptor. We return the first descriptor with matching IP and port.


http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

PS3, Line 95: 
> do we have a check anywhere that this flag is set, and give an error messag
It's set to default value 29000 if the user doesn't specify it during startup. We can potentially do a check for the validity of its value but I don't see the need to make an exception for it if we aren't already testing the values of other ports or whether the port is already in use.


Line 241:       ADD_TIMER(schedule->summary_profile(), "ComputeScanRangeAssignmentTimer");
> isn't this dcheck really saying that if LookupBackendDesc() returned nullpt
Done. Note that in release builds, we will just return the wrong descriptor. Is it better to crash or should we trust our testing to have covered it ?

I suppose similar questions can be asked about other DCHECKs in the existing code base.


Line 711: #endif
> I don't understand that. isn't this just copying the shared_ptr, not the Ba
The way the update happens is that it overwrites the shared_ptr "executors_config_" atomically to point to the new copy. So, if you call GetExecutorsConfig() twice, you can potentially get back two different pointers. Checking out a copy here (with shared_ptr) makes sure that we retain reference to the old copy even after "executors_config_" has been overwritten.


http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 410:   // IP address + port of the KRPC based services on the destination
> does this have to be resolved IP as well?
Done


http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

Line 72:   // IP address + port of KRPC based services on this backend
> this one has to be a resolved IP, right? let's make that clear.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes