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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
