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,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:

(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 <henry@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message