impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Date Mon, 28 Aug 2017 19:16:02 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 3:

(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' perspective.  maybe
it should say "KRPC-based ImpalaInternalService is exported"? or will this port be used outside
of that?


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 (like in LookupBackendIp())?


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

PS3, Line 95: FLAGS_krpc_port
do we have a check anywhere that this flag is set, and give an error message if not?


Line 96:     local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr);
why does that have to get saved in the backend descriptor? since the port is constant and
we already have the ip, why not just use those fields rather than creating another?


Line 241:   DCHECK(desc != nullptr);
isn't this dcheck really saying that if LookupBackendDesc() returned nullptr, then we better
have host == local_backend_descriptor_.address? If so, seems more direct to just DCHECK that
instead of make it an if-stmt.


Line 711:   // between ComputeScanRangeAssignment() and ComputeFragmentExecParams().
I don't understand that. isn't this just copying the shared_ptr, not the BackendConfig itself?
so how does it avoid the inconsistency?


http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1660:         MakeNetworkAddress(ip, FLAGS_krpc_port));
another reason to not add krpc_svc_address but instead just generate it from ip and the krpc_port
on the fly.


-- 
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: 3
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