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 <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
