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, 18 Jan 2017 23:42:22 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 2:

File be/src/rpc/CMakeLists.txt:

where is this defined?
File be/src/rpc/rpc-builder.h:

Line 40: /// auto rpc = RpcBuilder<MyServiceProxy>::MakeRpc(address, rpc_mgr)
use actual type, not auto

Line 41: ///     .SetTimeout(timeout)
it feels like the service itself should have reasonable defaults, instead of having to set
them for each rpc invocation individually.

Line 84:     Status Execute(const F& func, const REQ& req, RESP* resp) {
how about making this a static function in the proxy class instead? same for the next one.
File be/src/rpc/rpc-mgr.h:

Line 18: #ifndef IMPALA_RPC_RPC_MGR_H
since this is part of the runtime system, let's move it under /runtime.

Line 21: #include "kudu/rpc/messenger.h"
do we really need all of these includes here, or can some of those move into the -inline.h
or .cc?

Line 45: /// An RpcMgr manages 0 or more services: RPC interfaces that are a collection of
RpcServiceMgr then?

Line 77: /// Each service and proxy interacts with the IO system via a shared pool of 'reactor'
what is 'the io system'?

Line 122:   Status GetProxy(const TNetworkAddress& address, std::unique_ptr<P>*
proxy) {
move to -inline.h

Line 145:     gscoped_ptr<kudu::rpc::ServiceIf> service_if;
how widely does this .h file get included? if it's not super-rare, let's try to move implementation
details out of it.

our .h includes are a real problem for build speed.

Line 158:     ServiceRegistration(ServiceRegistration&& other)
why is this needed?

Line 178:   const scoped_refptr<kudu::rpc::ResultTracker> tracker_;
why not inline?

Line 183:   std::shared_ptr<kudu::rpc::Messenger> messenger_;
explain why this needs to be a shared_ptr, ie, who it's shared with.

Line 190: Status RpcMgr::RegisterService(
move to -inline.h

Line 198:   service_registrations_.push_back(std::move(registration));
emplace_back instead
File be/src/rpc/thrift-util.h:

Line 143:   return serializer.Serialize(thrift, proto->mutable_thrift_struct());
protos have a standard function called mutable_thrift_struct()?
File be/src/statestore/

Line 36: #include "statestore/statestore.proxy.h"
are these generated?

Line 76: class StatestoreSubscriberImpl : public rpc::StatestoreSubscriberIf {
where does this come from?

Line 79:       const scoped_refptr<kudu::rpc::ResultTracker> tracker)
why not const&?

Line 89:     if (status.ok()) {
if this is !ok(), wouldn't thrift_response.status be OK?

Line 91:       if (thrift_request.__isset.registration_id) {
why isn't !__isset an error?

Line 102:     context->RespondSuccess();
what does this mean if there was a deserialization error?

Line 179:   auto rpc = RpcBuilder<StatestoreServiceProxy>::MakeRpc<RegisterSubscriberRequestPB,
no auto

Line 180:       RegisterSubscriberResponsePB>(statestore_address_, rpc_mgr_);
repeating all these template params feels redundant. what does this look like if MakeRpc()
makes the actual call, not produces a data structure?

Line 210:     RETURN_IF_ERROR(rpc_mgr_->RegisterService<StatestoreSubscriberImpl>(1,
10, &impl));
can't it deduce the type param?

also, since -Impl is the service, why not name it -Svc or something like that? that would
make it easier to tie it back to the service definition.

re 1 and 10: make them flags, unless there's a good reason they shouldn't be changed.
File be/src/statestore/statestore-subscriber.h:

Line 266
what happened here?

Line 73:       RpcMgr* rpc_mgr, MetricGroup* metrics);
explain param

Line 111:   /// heartbeat service, as well as a thread to check for failure and
there is no heartbeat service
File be/src/statestore/

Line 132:   void set_statestore(Statestore* statestore) { statestore_ = statestore; }
why not make this a c'tor param

Line 366:     if (ShouldExit()) return Status("Statestore is shutting down");
if you care about this, shouldn't it be called before the if?

Line 734:   }
why the logic change there? (and why schedule another update if you just unregistered the

Line 779: Status Statestore::MainLoop() {
move the body into Start() and remove this function
File be/src/statestore/statestore.h:

Line 109:   /// The main processing loop. Blocks until the exit flag is set. Does not call
who calls this and does it need to be public?

Line 403:   boost::scoped_ptr<RpcMgr> rpc_mgr_;
why not inline?
File be/src/statestore/statestore.proto:

Line 19: package impala.rpc;
why a different namespace?

Line 21: //////////////////////////////////////////////////////////////////////////////////////////
what for?

Line 22: 
leave note explaining what .thrift file this is paired up with

Line 23: message UpdateStateRequestPB { required bytes thrift_struct = 1; }
these typically end in Msg

also, since these are all wrappers, let's define a single thrift wrapper msg, then you can
get rid of one template param in the serialize/deserialize util functions.
File be/src/testutil/

Line 163:   // boost::shared_ptr<TProcessor> processor(
remove commented out code
File be/src/util/collection-metrics.h:

Line 234:   int64_t count() const { return boost::accumulators::count(acc_); }
this isn't truly a getter
File common/thrift/StatestoreService.thrift:

Line 147
leave a note explaining where the service definition that uses the structs here can be found.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message