Henry Robinson has posted comments on this change.
Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore
services to KRPC
......................................................................
Patch Set 4:
(18 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
Done. clang-format isn't great with lambdas, to my eye.
Line 79: req.port++;
> what's that for?
I'll comment. It's just to show that the request actually did something.
Line 129: ASSERT_EQ(retries, 3);
> define 3 as constant in rpc.h
Done
Line 166: .ok());
> is there some error code that indicates timeout?
Not that gets propagated into our Status object - there is room for improvement
in all our Kudu integration here. I can check the error msg though.
Line 188: LOG(INFO) << "RPC processed: " << total;
> remove
Done
Line 208: RpcController* controller = new RpcController();
> please use the rpc class here, since that's what we expect to be using norm
We can't here because Rpc doesn't (in this patch) have async support, and
that's what's needed here if we want to avoid spawning a ton of threads.
In the ImpalaInternalService patch there's lots of need for the async path
(which is quite complex), so I'll defer changing this test until then.
Line 210: proxy->PingAsync(PingRequestPb(), resp, controller, [resp,
controller]() {
> i find this formatting makes it hard to decipher this statement. move lambd
Done. Agree that we're still looking for good formatting for lambdas.
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 pa
Init() is required for RpcMgr to do anything - manage proxies or services. The
reactor threads are started in Init() because they're common to both use cases.
StartServices() is required only if you're going to run some services. So we
shouldn't force callers to pass a parameter to Init() that might not be used.
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?
At least for this patch, yes, because of the use for it for async RPCs.
http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
Line 91: ///
> then let's check for a runtime error (and return an error status or log the
I added a DCHECK in those methods as a compromise - I don't think we should
exit the process from those methods, and returning an error would make the API
much more unpleasant to use. This way our programming errors get flagged in
debug builds, but configuration errors get good error messages.
Line 96: return Status(
> same here.
Done
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
> i think the correct terminology would be 'a single rpc'. or you can write i
I've written 'remote method invocation' here to be clear that I'm not
circularly referring to the class itself.
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
Done
Line 116: if (!IsRetryableError(controller.status(),
controller.error_response())) {
> should this be isretryable(controller)? ie, will there ever be a call where
Done
Line 161: static bool IsRetryableError(
> comment
Done
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?
That trips up the proto compiler / cmake tooling.
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
How come? subscribers_ is a map, so the type of s is an map entry, which I
thought was an ok use case for 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?
clang-format can't handle comments in proto files. I missed reverting this one.
We need to exclude protos from formatting.
--
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
