impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
Date Thu, 26 Jan 2017 04:05:06 GMT
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:

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message