[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

PS3, Line 41: 0
what does it mean to manage 0 services?


PS3, Line 49: CLIENTS
why define new terminology? isn't this just "proxy" in KuduRPC speak?


PS3, Line 52: GetProxy
so I guess RpcMgr deals with both the server and client side of things?  As far 
as KuduRPC terminology goes, is "service" a thing specific to the server side 
of RPCs, or is it both server and client side (maybe it's a collection of RPC 
"prototypes")?  Does KuduRPC allow a process to instantiate a proxy without 
starting the corresponding server side service?  I'm not suggesting we have to 
separate these out, but trying to understand the Kudu terminology and KuduRPC 
interface.


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:

PS3, Line 34: proxy
I was confused about this whole class until I read rpc-mgr.h since I didn't 
really know what a proxy was until then. I think this needs to be 
cross-referenced, or something.


PS3, Line 36: Clients
over in rpc-mgr.h, you seemed to use "client" as a synonym for proxy, but where 
it sounds like the caller of the proxy (i.e. the caller of Execute()).


PS3, Line 36: single RPC instance
what is a "single RPC instance"?  A particular remote function, or a particular 
invocation of a remote function function or something else?


PS3, Line 48: Rpc
I was confused about this class at first since the name sounds like it's the 
RPC itself, but it isn't. This is really a wrapper around the RPC (which the 
proxy executes) to give us some retry logic, right?  Maybe we should call this 
RpcWrapper or something more specific?

Though I'm surprised KuduRPC layer itself doesn't handle this for us.  Does 
Kudu have a similar wrapper? Or how do they handle this retry logic?


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

Re: [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Daniel Hecht]

I think they could see it because they were "reviewers". Once I added
myself (via posting a comment), it showed up.

On Wed, Jan 25, 2017 at 7:21 PM Henry Robinson  wrote:

> Hm, I originally tried to upload it as a draft, but it published the patch
> e-mail to everyone (and I decided it was ok for review). Marcel and Sailesh
> have successfully published comments on PS3, so it seems that it was
> visible.
>
> I just 'published' the draft. Hopefully that fixes any remaining
> inconsistencies.
>
> On 25 January 2017 at 19:18, Henry Robinson (Code Review) <
> ger...@cloudera.org> wrote:
>
> Henry Robinson has uploaded a new patch set (#3).
>
> Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore
> services to KRPC
> ..
>
> IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
>
> This patch adds the core abstractions necessary for using Kudu RPC for
> Impala's backend services. The StatestoreService and
> StatestoreSubscriberService are both ported to KRPC to demonstrate how
> the new RPC framework operates.
>
> The main new class is RpcMgr, which is responsible for both services run
> by a daemon and for obtaining clients to remote services.
>
> Also done:
>  * Utility code to convert from a Thrift structure to a Protobuf
>wrapper. Thrift RPCs do not need to be ported to Protobuf to work.
>  * Added more build support for Protobuf generation. All library targets
>now depend on the Protobuf generation step, just as they do Thrift.
>  * thrift-server-test is rewritten to use Beeswax as a test service,
>since the old StatestoreService has been removed.
>
> TESTING:
>  * Impala's test suite passes.
>  * statestore-test has been rewritten to use new statestore code.
>  * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.
>
> TODO:
>  * Enable SSL and Kerberos support (already in Kudu's imported
>libraries)
>  * Rewrite test_statestore.py in C++. We don't have protobuf support in
>Python.
>
> Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
> ---
> M be/generated-sources/gen-cpp/CMakeLists.txt
> M be/src/catalog/CMakeLists.txt
> M be/src/catalog/catalog-server.cc
> M be/src/catalog/catalog-server.h
> M be/src/codegen/CMakeLists.txt
> M be/src/common/CMakeLists.txt
> M be/src/exec/CMakeLists.txt
> M be/src/exec/catalog-op-executor.cc
> M be/src/exec/hdfs-avro-scanner-test.cc
> M be/src/exec/kudu-util.h
> M be/src/experiments/CMakeLists.txt
> M be/src/exprs/CMakeLists.txt
> M be/src/rpc/CMakeLists.txt
> A be/src/rpc/common.proto
> A be/src/rpc/rpc-mgr-test.cc
> A be/src/rpc/rpc-mgr.cc
> A be/src/rpc/rpc-mgr.h
> A be/src/rpc/rpc-mgr.inline.h
> A be/src/rpc/rpc.h
> A be/src/rpc/rpc_test.proto
> M be/src/rpc/thrift-server-test.cc
> M be/src/rpc/thrift-util.h
> M be/src/runtime/CMakeLists.txt
> M be/src/runtime/buffered-tuple-stream-test.cc
> M be/src/runtime/bufferpool/CMakeLists.txt
> M be/src/runtime/exec-env.cc
> M be/src/runtime/exec-env.h
> M be/src/scheduling/CMakeLists.txt
> M be/src/service/CMakeLists.txt
> M be/src/service/query-exec-state.cc
> M be/src/statestore/CMakeLists.txt
> M be/src/statestore/statestore-subscriber.cc
> M be/src/statestore/statestore-subscriber.h
> A be/src/statestore/statestore-test.cc
> M be/src/statestore/statestore.cc
> M be/src/statestore/statestore.h
> A be/src/statestore/statestore.proto
> M be/src/statestore/statestored-main.cc
> M be/src/testutil/CMakeLists.txt
> M be/src/transport/CMakeLists.txt
> M be/src/udf/CMakeLists.txt
> M be/src/udf_samples/CMakeLists.txt
> M be/src/util/CMakeLists.txt
> M be/src/util/collection-metrics.h
> M common/thrift/CMakeLists.txt
> M common/thrift/StatestoreService.thrift
> M tests/statestore/test_statestore.py
> 47 files changed, 1,771 insertions(+), 749 deletions(-)
>
>
>   git pull ssh://gerrit.cloudera.org:29418/Impala-ASF
> refs/changes/20/5720/3
> --
> To view, visit http://gerrit.cloudera.org:8080/5720
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: newpatchset
> Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
> Gerrit-PatchSet: 3
> 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 
>
> --
> You received this message because you are subscribed to the Google Groups
> "impala-cr" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to impala-cr+unsubscr...@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>
>
>
>
> --
> Henry 

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Marcel Kornacker (Code Review)]

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(),
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

Re: [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson]

Hm, I originally tried to upload it as a draft, but it published the patch
e-mail to everyone (and I decided it was ok for review). Marcel and Sailesh
have successfully published comments on PS3, so it seems that it was
visible.

I just 'published' the draft. Hopefully that fixes any remaining
inconsistencies.

On 25 January 2017 at 19:18, Henry Robinson (Code Review) <
ger...@cloudera.org> wrote:

> Henry Robinson has uploaded a new patch set (#3).
>
> Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore
> services to KRPC
> ..
>
> IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
>
> This patch adds the core abstractions necessary for using Kudu RPC for
> Impala's backend services. The StatestoreService and
> StatestoreSubscriberService are both ported to KRPC to demonstrate how
> the new RPC framework operates.
>
> The main new class is RpcMgr, which is responsible for both services run
> by a daemon and for obtaining clients to remote services.
>
> Also done:
>  * Utility code to convert from a Thrift structure to a Protobuf
>wrapper. Thrift RPCs do not need to be ported to Protobuf to work.
>  * Added more build support for Protobuf generation. All library targets
>now depend on the Protobuf generation step, just as they do Thrift.
>  * thrift-server-test is rewritten to use Beeswax as a test service,
>since the old StatestoreService has been removed.
>
> TESTING:
>  * Impala's test suite passes.
>  * statestore-test has been rewritten to use new statestore code.
>  * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.
>
> TODO:
>  * Enable SSL and Kerberos support (already in Kudu's imported
>libraries)
>  * Rewrite test_statestore.py in C++. We don't have protobuf support in
>Python.
>
> Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
> ---
> M be/generated-sources/gen-cpp/CMakeLists.txt
> M be/src/catalog/CMakeLists.txt
> M be/src/catalog/catalog-server.cc
> M be/src/catalog/catalog-server.h
> M be/src/codegen/CMakeLists.txt
> M be/src/common/CMakeLists.txt
> M be/src/exec/CMakeLists.txt
> M be/src/exec/catalog-op-executor.cc
> M be/src/exec/hdfs-avro-scanner-test.cc
> M be/src/exec/kudu-util.h
> M be/src/experiments/CMakeLists.txt
> M be/src/exprs/CMakeLists.txt
> M be/src/rpc/CMakeLists.txt
> A be/src/rpc/common.proto
> A be/src/rpc/rpc-mgr-test.cc
> A be/src/rpc/rpc-mgr.cc
> A be/src/rpc/rpc-mgr.h
> A be/src/rpc/rpc-mgr.inline.h
> A be/src/rpc/rpc.h
> A be/src/rpc/rpc_test.proto
> M be/src/rpc/thrift-server-test.cc
> M be/src/rpc/thrift-util.h
> M be/src/runtime/CMakeLists.txt
> M be/src/runtime/buffered-tuple-stream-test.cc
> M be/src/runtime/bufferpool/CMakeLists.txt
> M be/src/runtime/exec-env.cc
> M be/src/runtime/exec-env.h
> M be/src/scheduling/CMakeLists.txt
> M be/src/service/CMakeLists.txt
> M be/src/service/query-exec-state.cc
> M be/src/statestore/CMakeLists.txt
> M be/src/statestore/statestore-subscriber.cc
> M be/src/statestore/statestore-subscriber.h
> A be/src/statestore/statestore-test.cc
> M be/src/statestore/statestore.cc
> M be/src/statestore/statestore.h
> A be/src/statestore/statestore.proto
> M be/src/statestore/statestored-main.cc
> M be/src/testutil/CMakeLists.txt
> M be/src/transport/CMakeLists.txt
> M be/src/udf/CMakeLists.txt
> M be/src/udf_samples/CMakeLists.txt
> M be/src/util/CMakeLists.txt
> M be/src/util/collection-metrics.h
> M common/thrift/CMakeLists.txt
> M common/thrift/StatestoreService.thrift
> M tests/statestore/test_statestore.py
> 47 files changed, 1,771 insertions(+), 749 deletions(-)
>
>
>   git pull ssh://gerrit.cloudera.org:29418/Impala-ASF
> refs/changes/20/5720/3
> --
> To view, visit http://gerrit.cloudera.org:8080/5720
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: newpatchset
> Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
> Gerrit-PatchSet: 3
> 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 
>
> --
> You received this message because you are subscribed to the Google Groups
> "impala-cr" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to impala-cr+unsubscr...@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>



-- 
Henry Robinson
Software Engineer
Cloudera
415-994-6679

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new patch set (#3).

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..

IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

This patch adds the core abstractions necessary for using Kudu RPC for
Impala's backend services. The StatestoreService and
StatestoreSubscriberService are both ported to KRPC to demonstrate how
the new RPC framework operates.

The main new class is RpcMgr, which is responsible for both services run
by a daemon and for obtaining clients to remote services.

Also done:
 * Utility code to convert from a Thrift structure to a Protobuf
   wrapper. Thrift RPCs do not need to be ported to Protobuf to work.
 * Added more build support for Protobuf generation. All library targets
   now depend on the Protobuf generation step, just as they do Thrift.
 * thrift-server-test is rewritten to use Beeswax as a test service,
   since the old StatestoreService has been removed.

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code.
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)
 * Rewrite test_statestore.py in C++. We don't have protobuf support in
   Python.

Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/codegen/CMakeLists.txt
M be/src/common/CMakeLists.txt
M be/src/exec/CMakeLists.txt
M be/src/exec/catalog-op-executor.cc
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/kudu-util.h
M be/src/experiments/CMakeLists.txt
M be/src/exprs/CMakeLists.txt
M be/src/rpc/CMakeLists.txt
A be/src/rpc/common.proto
A be/src/rpc/rpc-mgr-test.cc
A be/src/rpc/rpc-mgr.cc
A be/src/rpc/rpc-mgr.h
A be/src/rpc/rpc-mgr.inline.h
A be/src/rpc/rpc.h
A be/src/rpc/rpc_test.proto
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
M be/src/service/CMakeLists.txt
M be/src/service/query-exec-state.cc
M be/src/statestore/CMakeLists.txt
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
A be/src/statestore/statestore-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A be/src/statestore/statestore.proto
M be/src/statestore/statestored-main.cc
M be/src/testutil/CMakeLists.txt
M be/src/transport/CMakeLists.txt
M be/src/udf/CMakeLists.txt
M be/src/udf_samples/CMakeLists.txt
M be/src/util/CMakeLists.txt
M be/src/util/collection-metrics.h
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
M tests/statestore/test_statestore.py
47 files changed, 1,771 insertions(+), 749 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 3
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 

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 3:

Hmm, after posting that comment, patch set 3 showed up for me... as a draft. 
Henry, did you mean to "downgrade" this into a draft?

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

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

I got an email about patch set 3, but I don't see patch sets 1 & 2.  Anyone 
have an idea what's going on?

-- 
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: 2
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: No

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Marcel Kornacker (Code Review)]

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:

(6 comments)

some preliminary comments. i'll do a thorough pass in a bit.

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

Line 18: #ifndef IMPALA_RPC_RPC_MGR_H
> Will postpone for now. There's a dependency on security code which currentl
what do you mean by 'hasn't been integrated yet'?


Line 183
> Done
?


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc_test.proto
File be/src/rpc/rpc_test.proto:

Line 18: package impala;
add comment describing what this is used for


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

Line 76: 
> rpc::StatestoreSubscriberIf comes from statestore.service.h, which is auto-
this could actually help with readability: adding a nested namespace for 
generated code. so something like generated::bla.

thoughts?


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

Line 77: class StatestoreSubscriberImpl : public StatestoreSubscriberIf {
calling it 'impl' isn't super-meaningful (impl of what?). would be nice to come 
up with a standard name for the service shims. ServiceImpl?


Line 174:   auto rpc = Rpc::Make(statestore_address_, 
rpc_mgr_);
definitely an improvement.


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

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

PS3, Line 52: 4
> Could be left as future work, but just wanted to bring it up. Do you think 
Yeah, this should be plumbed through, will do that.


PS3, Line 64: service_pool->Init
> Safer to Init() after calling messenger_->RegisterService() ?
Hm, why do you think so? Seems more safe to get the threads running before the 
service is 'exposed' via RegisterService() (even though StartServices() is 
where requests are actually allowed to start arriving).


PS3, Line 74: int32_t num_acceptor_threads
> If this is already a user controlled flag, why pass it in as a parameter?
Because the caller may not want to respect the flag's value (maybe for tests or 
something).


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:

PS3, Line 90: func
> As I understand it currently, if 'func' is the asynchronous variant of the 
You simply can't call asynchronous functions with Execute() - there's no 
callback provided to the method invocation on line 108.

ExecuteAsync() is in the next patch. There's no use cases for it in this one.


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

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

PS3, Line 52: 4
Could be left as future work, but just wanted to bring it up. Do you think this 
value should be passed as an argument into Init(), since we may identify that 
we need a different number of reactor threads for different daemon types?


PS3, Line 64: service_pool->Init
Safer to Init() after calling messenger_->RegisterService() ?


PS3, Line 74: int32_t num_acceptor_threads
If this is already a user controlled flag, why pass it in as a parameter?


http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:

PS3, Line 90: func
As I understand it currently, if 'func' is the asynchronous variant of the 
remote method, we could end up with some sort of runtime error in L108, as 
we're not calling it with a callback argument.

I don't know what's the best way to do this, but should we disallow calling 
asynchronous functions with Execute() somehow? And introduce an ExecuteAsync() 
variant?


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

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Juan Yu (Code Review)]

Juan Yu has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.proto
File be/src/statestore/statestore.proto:

PS2, Line 41: UpdateState
I feel 'Topic' is more clear, like UpdateTopic, UpdateTopicRequestPB, 
UpdateTopicResponsePB


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
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

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

PS2, Line 60: inline Status FromKuduStatus
Leave a TODO if we're planning on standardizing the Status class.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-builder.h
File be/src/rpc/rpc-builder.h:

PS2, Line 109: if (!err || !err->has_code()
 : || err->code() != 
kudu::rpc::ErrorStatusPB::ERROR_SERVER_TOO_BUSY) {
 :   return FromKuduStatus(controller.status());
 : }
Should we add a comment stating what sort of error/error codes don't warrant a 
retry?

Or something like "Retry RPC if the server is too busy, else return success or 
error status."


PS2, Line 131:   DeserializeThriftFromProtoWrapper(response_proto, resp);
Check return val?


PS2, Line 157: MakeRpc
This function name sounds like it's executing the RPC, akin to ExecuteRPC(). 
Would something like MakeRpcObject be a more suitable name?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

PS2, Line 143: ServiceRegistration
Do you think this struct is really very necessary? We can do the service 
registration with the Messenger in RpcMgr::RegisterService() and just save the 
service_pool objects. We can then call Init() on these service_pool objects in 
StartServices().

If you were thinking this would be useful information to add to the debug 
webpages, we can get the same info from the ServicePool object itself. Is there 
any other use you foresee for this object?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

PS2, Line 101: SerializeThriftToProtoWrapper(_response, response);
Check the return value?


PS2, Line 114: SerializeThriftToProtoWrapper(_response, response);
Same here.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS2, Line 772: 32, 1024
Should we make these const uints for clarity?


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(41 comments)

http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/CMakeLists.txt
File be/src/rpc/CMakeLists.txt:

Line 36: KRPC_GENERATE(RPC_TEST_SVC_PROTO_SRCS RPC_TEST_SVC_PROTO_HDRS
where is this defined?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-builder.h
File be/src/rpc/rpc-builder.h:

Line 40: /// auto rpc = RpcBuilder::MakeRpc(address, rpc_mgr)
use actual type, not auto


Line 41: /// .SetTimeout(timeout)
it feels like the service itself should have reasonable defaults, instead of 
having to set them for each rpc invocation individually.


Line 84: Status Execute(const F& func, const REQ& req, RESP* resp) {
how about making this a static function in the proxy class instead? same for 
the next one.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

Line 18: #ifndef IMPALA_RPC_RPC_MGR_H
since this is part of the runtime system, let's move it under /runtime.


Line 21: #include "kudu/rpc/messenger.h"
do we really need all of these includes here, or can some of those move into 
the -inline.h or .cc?


Line 45: /// An RpcMgr manages 0 or more services: RPC interfaces that are a 
collection of remotely
RpcServiceMgr then?


Line 77: /// Each service and proxy interacts with the IO system via a shared 
pool of 'reactor'
what is 'the io system'?


Line 122:   Status GetProxy(const TNetworkAddress& address, std::unique_ptr* 
proxy) {
move to -inline.h


Line 145: gscoped_ptr service_if;
how widely does this .h file get included? if it's not super-rare, let's try to 
move implementation details out of it.

our .h includes are a real problem for build speed.


Line 158: ServiceRegistration(ServiceRegistration&& other)
why is this needed?


Line 178:   const scoped_refptr tracker_;
why not inline?


Line 183:   std::shared_ptr messenger_;
explain why this needs to be a shared_ptr, ie, who it's shared with.


Line 190: Status RpcMgr::RegisterService(
move to -inline.h


Line 198:   service_registrations_.push_back(std::move(registration));
emplace_back instead


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/thrift-util.h
File be/src/rpc/thrift-util.h:

Line 143:   return serializer.Serialize(thrift, proto->mutable_thrift_struct());
protos have a standard function called mutable_thrift_struct()?


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:

Line 36: #include "statestore/statestore.proxy.h"
are these generated?


Line 76: class StatestoreSubscriberImpl : public rpc::StatestoreSubscriberIf {
where does this come from?


Line 79:   const scoped_refptr tracker)
why not const&?


Line 89: if (status.ok()) {
if this is !ok(), wouldn't thrift_response.status be OK?


Line 91:   if (thrift_request.__isset.registration_id) {
why isn't !__isset an error?


Line 102: context->RespondSuccess();
what does this mean if there was a deserialization error?


Line 179:   auto rpc = 
RpcBuilder::MakeRpc(statestore_address_, rpc_mgr_);
repeating all these template params feels redundant. what does this look like 
if MakeRpc() makes the actual call, not produces a data structure?


Line 210: 
RETURN_IF_ERROR(rpc_mgr_->RegisterService(1, 10, 
));
can't it deduce the type param?

also, since -Impl is the service, why not name it -Svc or something like that? 
that would make it easier to tie it back to the service definition.

re 1 and 10: make them flags, unless there's a good reason they shouldn't be 
changed.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.h
File be/src/statestore/statestore-subscriber.h:

Line 266
what happened here?


Line 73:   RpcMgr* rpc_mgr, MetricGroup* metrics);
explain param


Line 111:   /// heartbeat service, as well as a thread to check for failure and
there is no heartbeat service


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

Line 132:   void set_statestore(Statestore* statestore) { statestore_ = 
statestore; }
why not make this a c'tor param


Line 366: if (ShouldExit()) return Status("Statestore is shutting down");
if you care about this, shouldn't it be called before the if?


Line 734:   }
why the logic change there? (and why schedule another update if you just 
unregistered the subscriber?)


Line 779: Status Statestore::MainLoop() {
move the body into Start() and remove this function


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

Line 109:   /// The main processing loop. Blocks until the exit flag is set. 
Does 

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

I'll start this review now.

-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5720/2/tests/statestore/test_statestore.py
File tests/statestore/test_statestore.py:

Line 18: # TODO(KRPC): Re-enable.
> Or theoretically (meaning I haven't tried this) you could add a pytest skip
I expect this file to get removed completely as part of this commit.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5720/2/tests/statestore/test_statestore.py
File tests/statestore/test_statestore.py:

Line 18: # TODO(KRPC): Re-enable.
> Instead of adding a # on every line, I would suggest to add ''' (three sing
Or theoretically (meaning I haven't tried this) you could add a pytest skipif 
mark at the module (global) scope.

   pytestmark = pytest.mark.skipif(reason="All tests to be rewritten in C++")


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5720/2/tests/statestore/test_statestore.py
File tests/statestore/test_statestore.py:

Line 18: # TODO(KRPC): Re-enable.
Instead of adding a # on every line, I would suggest to add ''' (three single 
quotes) at the beginning and at the end.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new patch set (#2).

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..

IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

This patch adds the core abstractions necessary for using Kudu RPC for
Impala's backend services. The StatestoreService and
StatestoreSubscriberService are both ported to KRPC to demonstrate how
the new RPC framework operates.

The main new class is RpcMgr, which is responsible for both services run
by a daemon and for obtaining clients to remote services.

Also done:
 * Utility code to convert from a Thrift structure to a Protobuf
   wrapper. Thrift RPCs do not need to be ported to Protobuf to work.
 * Added more build support for Protobuf generation. All library targets
   now depend on the Protobuf generation step, just as they do Thrift.
 * thrift-server-test is rewritten to use Beeswax as a test service,
   since the old StatestoreService has been removed.

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code.
 * rpc-mgr-test unit-tests the RpcMgr and RpcBuilder classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)
 * Rewrite test_statestore.py in C++. We don't have protobuf support in
   Python.

Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/codegen/CMakeLists.txt
M be/src/common/CMakeLists.txt
M be/src/exec/CMakeLists.txt
M be/src/exec/catalog-op-executor.cc
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/kudu-util.h
M be/src/experiments/CMakeLists.txt
M be/src/exprs/CMakeLists.txt
M be/src/rpc/CMakeLists.txt
A be/src/rpc/rpc-builder.h
A be/src/rpc/rpc-mgr-test.cc
A be/src/rpc/rpc-mgr.cc
A be/src/rpc/rpc-mgr.h
A be/src/rpc/rpc_test.proto
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
M be/src/service/CMakeLists.txt
M be/src/service/query-exec-state.cc
M be/src/statestore/CMakeLists.txt
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A be/src/statestore/statestore.proto
M be/src/statestore/statestored-main.cc
M be/src/testutil/CMakeLists.txt
M be/src/testutil/in-process-servers.cc
M be/src/transport/CMakeLists.txt
M be/src/udf/CMakeLists.txt
M be/src/udf_samples/CMakeLists.txt
M be/src/util/CMakeLists.txt
M be/src/util/collection-metrics.h
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
M tests/statestore/test_statestore.py
46 files changed, 1,589 insertions(+), 786 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5720

Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore 
services to KRPC
..

IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC

This patch adds the core abstractions necessary for using Kudu RPC for
Impala's backend services. The StatestoreService and
StatestoreSubscriberService are both ported to KRPC to demonstrate how
the new RPC framework operates.

The main new class is RpcMgr, which is responsible for both services run
by a daemon and for obtaining clients to remote services.

Also done:
 * Utility code to convert from a Thrift structure to a Protobuf
   wrapper. Thrift RPCs do not need to be ported to Protobuf to work.
 * Added more build support for Protobuf generation. All library targets
   now depend on the Protobuf generation step, just as they do Thrift.
 * thrift-server-test is rewritten to use Beeswax as a test service,
   since the old StatestoreService has been removed.

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code.
 * rpc-mgr-test unit-tests the RpcMgr and RpcBuilder classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)
 * Rewrite test_statestore.py in C++. We don't have protobuf support in
   Python.

Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/catalog/CMakeLists.txt
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/codegen/CMakeLists.txt
M be/src/common/CMakeLists.txt
M be/src/exec/CMakeLists.txt
M be/src/exec/catalog-op-executor.cc
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/kudu-util.h
M be/src/experiments/CMakeLists.txt
M be/src/exprs/CMakeLists.txt
M be/src/rpc/CMakeLists.txt
A be/src/rpc/rpc-builder.h
A be/src/rpc/rpc-mgr-test.cc
A be/src/rpc/rpc-mgr.cc
A be/src/rpc/rpc-mgr.h
A be/src/rpc/rpc_test.proto
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/CMakeLists.txt
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/CMakeLists.txt
M be/src/service/CMakeLists.txt
M be/src/service/query-exec-state.cc
M be/src/statestore/CMakeLists.txt
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore-test.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A be/src/statestore/statestore.proto
M be/src/statestore/statestored-main.cc
M be/src/testutil/CMakeLists.txt
M be/src/testutil/in-process-servers.cc
M be/src/transport/CMakeLists.txt
M be/src/udf/CMakeLists.txt
M be/src/udf_samples/CMakeLists.txt
M be/src/util/CMakeLists.txt
M be/src/util/collection-metrics.h
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
M tests/statestore/test_statestore.py
46 files changed, 1,587 insertions(+), 786 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson