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,4672,4784}: Add RpcMgr and port Statestore services to KRPC
Date Sat, 28 Jan 2017 02:33:33 GMT
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:

File be/src/rpc/

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;

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?
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>*
do we want this to be public, since we have the rpc wrapper class?
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
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.
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
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(
File be/src/rpc/rpc_test.proto:

Line 1: // Licensed to the Apache Software Foundation (ASF) under one
why not rpc-test.proto?
File be/src/statestore/

Line 792:   for (auto s : subscribers_) ret.insert(s.first);
no auto
File be/src/statestore/statestore.proto:

Line 19: // common/thrift/StatestoreService.thrift for complementary Thrift data
why not a single line?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message