Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore
services to KRPC
......................................................................
Patch Set 4:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:
Line 65: RpcContext* ctx) { ctx->RespondSuccess(); })
put lambda on single line? i found the linebreak hard to parse
Line 79: req.port++;
what's that for?
Line 129: ASSERT_EQ(retries, 3);
define 3 as constant in rpc.h
Line 166: .ok());
is there some error code that indicates timeout?
Line 188: LOG(INFO) << "RPC processed: " << total;
remove
Line 208: RpcController* controller = new RpcController();
please use the rpc class here, since that's what we expect to be using normally.
Line 210: proxy->PingAsync(PingRequestPb(), resp, controller, [resp,
controller]() {
i find this formatting makes it hard to decipher this statement. move lambda to
single line?
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
Line 95: Status Init(int32_t num_reactor_threads);
why pass as a param here rather than in startservices? (or: why pass any
parameters to startservices instead of init?)
Line 123: Status GetProxy(const TNetworkAddress& address, std::unique_ptr<P>*
proxy);
do we want this to be public, since we have the rpc wrapper class?
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),
> It's a DNS subsystem call.
this doesn't sound lightweight anymore, and critically depending on additional
infrastructure also doesn't seem like a good idea.
as discussed in person just now: let's check with the field whether we know
whether they actually run nscd
http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
Line 68: Rpc& SetMaxAttempts(int32_t max_attempts) {
> It doesn't have to be, but I thought in general we tried to be explicit. Ca
we do if it's needed for correctness (ie, a storage type).
Line 91: ///
> Typo - should be <, not <=.
then let's check for a runtime error (and return an error status or log the
error and exit) where the flag is applied, not here.
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
PS4, Line 36: single RPC instance
> still confusing
i think the correct terminology would be 'a single rpc'. or you can write it
out if you think that avoids confusing it with the service methods.
Line 115: // Retry only if the remote returns ERROR_SERVER_TOO_BUSY.
Otherwise we fail fast.
i wouldn't embed the innards of isretryable() in this comment
Line 116: if (!IsRetryableError(controller.status(),
controller.error_response())) {
should this be isretryable(controller)? ie, will there ever be a call where
status and error_response don't come from a controller?
Line 161: static bool IsRetryableError(
comment
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/rpc/rpc_test.proto
File be/src/rpc/rpc_test.proto:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
why not rpc-test.proto?
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:
Line 792: for (auto s : subscribers_) ret.insert(s.first);
no auto
http://gerrit.cloudera.org:8080/#/c/5720/4/be/src/statestore/statestore.proto
File be/src/statestore/statestore.proto:
Line 19: // common/thrift/StatestoreService.thrift for complementary Thrift data
why not a single line?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: David Knupp <[email protected]>
Gerrit-Reviewer: Henry Robinson <[email protected]>
Gerrit-Reviewer: Juan Yu <[email protected]>
Gerrit-Reviewer: Marcel Kornacker <[email protected]>
Gerrit-Reviewer: Sailesh Mukil <[email protected]>
Gerrit-Reviewer: Taras Bobrovytsky <[email protected]>
Gerrit-HasComments: Yes
