impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
Date Wed, 25 Jan 2017 20:38:32 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

Patch Set 3:


some preliminary comments. i'll do a thorough pass in a bit.
File be/src/rpc/rpc-mgr.h:

Line 18: #ifndef IMPALA_RPC_RPC_MGR_H
> Will postpone for now. There's a dependency on security code which currentl
what do you mean by 'hasn't been integrated yet'?

Line 183
> Done
File be/src/rpc/rpc_test.proto:

Line 18: package impala;
add comment describing what this is used for
File be/src/statestore/

Line 76: 
> rpc::StatestoreSubscriberIf comes from statestore.service.h, which is auto-
this could actually help with readability: adding a nested namespace for generated code. so
something like generated::bla.

File be/src/statestore/

Line 77: class StatestoreSubscriberImpl : public StatestoreSubscriberIf {
calling it 'impl' isn't super-meaningful (impl of what?). would be nice to come up with a
standard name for the service shims. ServiceImpl?

Line 174:   auto rpc = Rpc<StatestoreServiceProxy>::Make(statestore_address_, rpc_mgr_);
definitely an improvement.

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