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: (23 comments) http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/common.proto File be/src/rpc/common.proto: Line 27: message ThriftWrapperPB { required bytes thrift_struct = 1; } a brief comment regarding the purpose would be good. regarding naming: if we want to stick with 'pb' let's at least make it '-Pb', not '-PB', which would be in line with the style guide. http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: Line 44: DEFINE_int32(num_acceptor_threads, 2, "Number of threads dedicated to accepting " why so low? Line 59: DCHECK(is_inited()) << "Must call Init() before RegisterService()"; dcheck that services haven't been started yet, or does that not matter? Line 62: new ServicePool(move(scoped_ptr), messenger_->metric_entity(), service_queue_depth); why not just pass service_ptr.release()? PS3, Line 64: service_pool->Init > Hm, why do you think so? Seems more safe to get the threads running before good point. if the init call fails, do we abort impalad startup? (i would assume so.) PS3, Line 74: int32_t num_acceptor_threads > Because the caller may not want to respect the flag's value (maybe for test so then move the flag to where it's used? http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: Line 64: /// RpcMgr::RegisterService() before RpcMgr::StartServices() is called. does init() go before register()? Line 111: /// Creates a new proxy for a remote service of type P at location 'address', and places might want to point out here what P needs to be (auto-generated from a service definition) http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.inline.h File be/src/rpc/rpc-mgr.inline.h: Line 39: kudu::HostPort(address.hostname, address.port).ResolveAddresses(&addresses), how expensive is this? Line 41: proxy->reset(new P(messenger_, addresses[0])); dcheck that addresses isn't empty? http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h File be/src/rpc/rpc.h: Line 34: /// concrete type of this class can create RPCs for a particular proxy type P. Clients of point out what's valid as a proxy type Line 51: /// 'RESP' (must both be protobuf). unclear what req and resp refer to Line 58: Rpc(const TNetworkAddress& remote, RpcMgr* mgr) : remote_(remote), mgr_(mgr) {} does this need to be public? Line 68: Rpc& SetMaxAttempts(int32_t max_attempts) { does this need to be specific (int32_t as opposed to int)? PS3, Line 90: func > You simply can't call asynchronous functions with Execute() - there's no ca point out in what way F is constrained Line 91: if (max_rpc_attempts_ <= 1) { why not dcheck in set..()? there's no reason this should be a runtime error. why is 1 an error? Line 96: if (max_rpc_attempts_ > 1 && retry_interval_ms_ <= 0) { same here. Line 112: if (!controller.status().IsRemoteError()) break; comment on significance of remote error. looks like you're getting a non-ok rpc return value back, but l116 seems to contradict that. i'm not sure how to assess the correctness of the following code. also, "return status;" does the same Line 143: /// TODO(KRPC): Warn on uninitialized timeout, or set meaningful default. why warn? if you don't set a timeout, it shouldn't time out. agreed about defaults. http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: Line 159: TNetworkAddress subscriber_address_; same address as the generic backend service, meaning it'll be removed once everything is switched over? http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: PS2, Line 101: const ThriftWrapperPB* request, ThriftWrapperPB* respons > That's a fair question, but what should we do with it? If we can't write th how will the caller fail to deserialize? can there be something partially serialized? http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: Line 207: return Status("--statestore_subscriber_num_svc_threads must be at least 2"); why not just bump it up to 2? http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/statestore/statestore-subscriber.h File be/src/statestore/statestore-subscriber.h: Line 128: TNetworkAddress subscriber_address_; wouldn't that be the same as the generic backend address? -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
