Dan Hecht has posted comments on this change. Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: PS3, Line 41: 0 what does it mean to manage 0 services? PS3, Line 49: CLIENTS why define new terminology? isn't this just "proxy" in KuduRPC speak? PS3, Line 52: GetProxy so I guess RpcMgr deals with both the server and client side of things? As far as KuduRPC terminology goes, is "service" a thing specific to the server side of RPCs, or is it both server and client side (maybe it's a collection of RPC "prototypes")? Does KuduRPC allow a process to instantiate a proxy without starting the corresponding server side service? I'm not suggesting we have to separate these out, but trying to understand the Kudu terminology and KuduRPC interface. http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h File be/src/rpc/rpc.h: PS3, Line 34: proxy I was confused about this whole class until I read rpc-mgr.h since I didn't really know what a proxy was until then. I think this needs to be cross-referenced, or something. PS3, Line 36: Clients over in rpc-mgr.h, you seemed to use "client" as a synonym for proxy, but where it sounds like the caller of the proxy (i.e. the caller of Execute()). PS3, Line 36: single RPC instance what is a "single RPC instance"? A particular remote function, or a particular invocation of a remote function function or something else? PS3, Line 48: Rpc I was confused about this class at first since the name sounds like it's the RPC itself, but it isn't. This is really a wrapper around the RPC (which the proxy executes) to give us some retry logic, right? Maybe we should call this RpcWrapper or something more specific? Though I'm surprised KuduRPC layer itself doesn't handle this for us. Does Kudu have a similar wrapper? Or how do they handle this retry logic? -- To view, visit http://gerrit.cloudera.org:8080/5720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
