impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class
Date Thu, 31 Aug 2017 20:44:59 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4670: Introduces RpcMgr class

Patch Set 3:

File be/src/rpc/

PS3, Line 62: int32_t payload_[PAYLOAD_SIZE];
'payload_' can be a private variable.

PS3, Line 60:   TNetworkAddress krpc_address_;
            :   RpcMgr rpc_mgr_;
            :   int32_t payload_[PAYLOAD_SIZE];
Member variable declarations should go below the functions definitions?

PS3, Line 75: SetupScanMemRequest
Could you add a brief function header?

PS3, Line 120: int32_t
nit: I think it would be better to either use int32_t or uint32_t consistently in this function.

PS3, Line 127:  
nit: add some delimiter here

PS3, Line 141: Start a second service which verifies the RPC payload is not corrupted.
nit: Test that a second service, that verifies the RPC payload is not corrupted, can be started.

PS3, Line 188: boost::bind
I'd prefer if we did this with a named lambda instead. What do you think?

PS3, Line 237: boost::bind
Same here
File be/src/rpc/rpc-mgr.h:

Line 95: /// port is configurable via FLAGS_acceptor_threads.
I think it would be good to add an example of the function calls a application developer would
need to make to send a RPC.


PS3, Line 129: The RPC layer will accept incoming
             :   /// connections but no service will be available.
Is that true? UnregisterServices() calls Messenger::Shutdown(), which calls AcceptorPool::Shutdown().

PS3, Line 171: }
} // namespace impala
File be/src/rpc/rpc-mgr.inline.h:

PS3, Line 43: }
} // namespace impala
File be/src/runtime/exec-env.h:

PS3, Line 32: namespace kudu {
            :   namespace client {
            :      class KuduClient;
            :   }
            : }
Nested namespaces shouldn't be indented, here and in other places.
File be/src/util/

PS3, Line 226: }
Not your change but:

} // namespace impala
File common/protobuf/rpc_test.proto:

Line 43
Is it not possible to use the existing Kudu protobuf structures for testing from the .proto
I'm fine with this too, but I just want to see if we can do away with some duplication.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message