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-4670: Introduces RpcMgr class
Date Wed, 20 Sep 2017 16:40:27 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4670: Introduces RpcMgr class
......................................................................


Patch Set 6:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

Line 87: /// Impala Status object.
maybe note that the returned error is always of type GENERAL, i.e. we don't try to translate
error codes.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

Line 30: using std::move;
why is that needed?


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

Line 76: ///
what is the thread safety story of this class? it looks like initializing it is not thread
safe, but then Shutdown() is thread safe? i.e. Shutdown() can be called while RPCs to/from
services are in flight?  let's briefly document whatever the thread-safety or lack of is.


PS6, Line 89: processed
that's talking about being processed by either acceptor or service, right? As opposed to being
processed by 'reactor'.  Could be clarified.


PS6, Line 89: FIFO fixed-size queue
is that queue per service? and is there a queue for connection requests as well? or is it
one big global queue?


PS6, Line 104: address
does that one have to be an IP addr or is hostname okay?


PS6, Line 115: being
begin


Line 133:   /// All acceptor and reactor threads within the messenger are also shut down.
what happens to stuff in flight, or should we not care for some reason?


PS6, Line 156: scoped_refptr
I guess using that is kinda required by krpc? What does it do? ServicePool destroys itself
based on a ref count or something?


PS6, Line 169: shared_ptr
I guess using shared_ptr here is required by the MessangerBuilder::Build(). Let's note that
in the comment.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

PS6, Line 135: krpc_port
does that still need to be exposed if we have krpc_address()? And looking ahead to your other
patch, it looks like this is used to construct the krpc_address, yet it comes from the krpc
address, so I'm confused.


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

Line 73
maybe this comment is still useful in the new code (minus the KRPC bits) in a slightly different
form:

Store our IP address, so that each ...


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc
File be/src/util/network-util.cc:

Line 120:   Sockaddr sock;
okay to ignore, but i think keeping the kudu:: namespace here makes it easier to understand
why we have this and where to look. (and since it's not wide-spread, it's not so bad to type
it out here).


PS6, Line 218: Sockaddr
same


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

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

Mime
View raw message