impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Date Tue, 29 Aug 2017 18:29:13 GMT
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

Mime
View raw message