impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(41 comments)

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/CMakeLists.txt
File be/src/rpc/CMakeLists.txt:

Line 36: KRPC_GENERATE(RPC_TEST_SVC_PROTO_SRCS RPC_TEST_SVC_PROTO_HDRS
where is this defined?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-builder.h
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.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h
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
remotely
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


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/thrift-util.h
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()?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.h
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


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

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
subscriber?)


Line 779: Status Statestore::MainLoop() {
move the body into Start() and remove this function


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

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


Line 403:   boost::scoped_ptr<RpcMgr> rpc_mgr_;
why not inline?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.proto
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.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:

Line 163:   // boost::shared_ptr<TProcessor> processor(
remove commented out code


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/util/collection-metrics.h
File be/src/util/collection-metrics.h:

Line 234:   int64_t count() const { return boost::accumulators::count(acc_); }
this isn't truly a getter


http://gerrit.cloudera.org:8080/#/c/5720/2/common/thrift/StatestoreService.thrift
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 http://gerrit.cloudera.org:8080/5720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message