You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/01/13 23:33:48 UTC

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

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 <he...@cloudera.com>

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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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:

(3 comments)

looks good, but let's talk about the nscd requirement today.

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

Line 166:                    .ok());
> Not that gets propagated into our Status object - there is room for improve
is there a todo somewhere?


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

Line 49: template <typename P>
move them into the class so they're naturally qualified?


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

Line 379:   subscribers_[0]->WaitForUpdates(1);
?


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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(&addresses),
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
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 13:

(3 comments)

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

PS13, Line 66: Sets the timeout for TCP writes and reads.
Isn't it actually end-to-end timeout of the async RPC according to RpcController::set_timeout().


PS13, Line 175: err
err != nullptr


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

PS11, Line 75: "The number of threads available to service "
             :                                              "incoming RPC requests"
nit: why is the indentation of the comment different from the lines above ?


-- 
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 7:

This patch is a rebase. The only outstanding issue is the performance of name lookups in the RPC.

-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#10).

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
51 files changed, 1,744 insertions(+), 1,084 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/10
-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 7:

(2 comments)

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

Line 55: template <typename P>
p is only used in Execute(). why not add p as a template parameter to that function and get rid of the templating for the class?


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

Line 379:   subscribers_[0]->WaitForFailure(statestore_.get());
> Done
what's done? i was wondering why the line disappeared.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 11:

(1 comment)

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

Line 112:   /// 'service_ptr' contains an interface implementation that will handle RPCs.
May help to comment that 'service_ptr' releases reference to the underlying object when this function returns.


-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has uploaded a new patch set (#16).

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/client-request-state.cc
M be/src/statestore/CMakeLists.txt
D be/src/statestore/statestore-service-client-wrapper.h
D be/src/statestore/statestore-subscriber-client-wrapper.h
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,733 insertions(+), 1,219 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/16
-- 
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 5:

(3 comments)

The next patch after this will be a rebase, but will contain no functional differences. This patch set only adds a comment.

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

Line 166:       .Execute(&PingServiceProxy::Ping, request, &response);
> is there a todo somewhere?
No - I think this is part of the larger synchronisation effort between the two sets of utility libraries.


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

Line 49: static constexpr int RPC_DEFAULT_MAX_ATTEMPTS = 3;
> move them into the class so they're naturally qualified?
I really couldn't make that work well with a templated class. I'll leave a TODO.


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

Line 379:   subscribers_[0]->WaitForFailure(statestore_.get());
> ?
Done


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/client-request-state.cc
M be/src/statestore/CMakeLists.txt
D be/src/statestore/statestore-service-client-wrapper.h
D be/src/statestore/statestore-subscriber-client-wrapper.h
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,733 insertions(+), 1,219 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/14
-- 
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#8).

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/buffered-tuple-stream-v2-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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
54 files changed, 1,748 insertions(+), 1,086 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/8
-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,740 insertions(+), 1,083 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/6
-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

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

Posted by Daniel Hecht <dh...@cloudera.com>.

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 <he...@cloudera.com> 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) <
> gerrit@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 <he...@cloudera.com>
> Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
> Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
> Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
> Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
> Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
> Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
>
> --
> 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+unsubscribe@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>
>
>
>
> --
> Henry Robinson
> Software Engineer
> Cloudera
> 415-994-6679
>
> --
> 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+unsubscribe@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>

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

Posted by Henry Robinson <he...@cloudera.com>.

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) <
gerrit@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 <he...@cloudera.com>
> Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
> Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
> Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
> Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
> Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
> Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
>
> --
> 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+unsubscribe@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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,727 insertions(+), 1,083 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/4
-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#13).

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/client-request-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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
51 files changed, 1,744 insertions(+), 1,091 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/13
-- 
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#11).

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/client-request-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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
51 files changed, 1,743 insertions(+), 1,091 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/11
-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

Patch Set 4:

(4 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
> It means that there are 0 services managed by the RpcMgr? Since the RpcMgr
Thanks, the "service and clients" duality here distinct from "service and proxy" duality in KuduRPC threw me off.

PS3, Line 49: Service
> I've tried to standardize a bit better. 'clients' and 'servers' are pretty
Thanks. Sure they are established nouns, but here we are talking about specific layer of the software stack so being consistent and always saying 'proxy' makes things clearer IMO. e.g. I wasn't sure if this comment was trying to introduce another concept or layer.

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

PS3, Line 36: Clients
> I've reworded rpc-mgr.h to standardize on Proxy. I'll write 'callers' here
now that rpc-mgr.h doesn't use client as top level terminology, saying "clients" here is okay (or callers is okay too but you didn't actually change it).

Though I still think single RPC instance is still confusing (but wasn't reworded).

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

--
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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 10:

Rebase, plus a couple of bug fixes.

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has abandoned this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Abandoned

-- 
To view, visit http://gerrit.cloudera.org:8080/5720
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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<StatestoreServiceProxy>::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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/exec/parquet-column-stats.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
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
54 files changed, 1,744 insertions(+), 1,087 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/7
-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

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

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

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<MyServiceProxy>::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<P>* proxy) {
move to -inline.h


Line 145:     gscoped_ptr<kudu::rpc::ServiceIf> 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<kudu::rpc::ResultTracker> tracker_;
why not inline?


Line 183:   std::shared_ptr<kudu::rpc::Messenger> 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<kudu::rpc::ResultTracker> 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<StatestoreServiceProxy>::MakeRpc<RegisterSubscriberRequestPB,
no auto


Line 180:       RegisterSubscriberResponsePB>(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<StatestoreSubscriberImpl>(1, 10, &impl));
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 not call Start().
who calls this and does it need to be public?


Line 403:   boost::scoped_ptr<RpcMgr> rpc_mgr_;
why not inline?


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

Line 19: package impala.rpc;
why a different namespace?


Line 21: //////////////////////////////////////////////////////////////////////////////////////////
what for?


Line 22: 
leave note explaining what .thrift file this is paired up with


Line 23: message UpdateStateRequestPB { required bytes thrift_struct = 1; }
these typically end in Msg

also, since these are all wrappers, let's define a single thrift wrapper msg, then you can get rid of one template param in the serialize/deserialize util functions.


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:

Line 163:   // boost::shared_ptr<TProcessor> processor(
remove commented out code


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

Line 234:   int64_t count() const { return boost::accumulators::count(acc_); }
this isn't truly a getter


http://gerrit.cloudera.org:8080/#/c/5720/2/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

Line 147
leave a note explaining where the service definition that uses the structs here can be found.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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:

(50 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.
I don't think that's imminent.


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

Line 36:   rpc-trace.cc
> where is this defined?
See cmake_modules/FindKRPC.cmake. Part of the KRPC library inclusion patch.

(https://github.com/henryr/Impala/blob/krpc/cmake_modules/FindKRPC.cmake#L25)


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

Line 40
> use actual type, not auto
I think it encourages readability here. The type of Rpc<MyServiceProxy>::Make() is Rpc<MyServiceProxy>. It seems repetitive to have that on both sides of the assignment.

The google style guide says:

 (Encouraged) ... long/cluttery type names, particularly when the type is clear from context.


Line 41
> it feels like the service itself should have reasonable defaults, instead o
Timeouts are a per-invocation construct in KRPC. They're set on the per-RPC RpcController object. 

I agree there should be a default timeout (it's better to occasionally timeout too early than to never return). I don't think it's necessary to change that per-service. However, there are cases where the timeout should be different for different methods on the same service.

For example, Statestore::Update() has a relatively long timeout (because it can take a long time). Statestore::Heartbeat() has a short one because it should execute very quickly. 

I think it makes sense to keep per-RPC timeouts, but have the Rpc class have a reasonable (60s?) timeout.


Line 84
> how about making this a static function in the proxy class instead? same fo
The proxy class is generated by the KRPC toolchain. It could be changed, but I'd prefer to share that code with Kudu for now.


PS2, Line 109: 
             : 
             : 
             : 
> Should we add a comment stating what sort of error/error codes don't warran
I think the method comment covers this. Let me know if you think it should be expanded upon.


PS2, Line 131: 
> Check return val?
Done


PS2, Line 157: 
> This function name sounds like it's executing the RPC, akin to ExecuteRPC()
I see your point, but I think the likelihood of confusion is small. Brevity is important for this method name particularly.


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.
Will postpone for now. There's a dependency on security code which currently lives in rpc/ that hasn't been integrated yet. After that let's take a view if this still belongs in runtime/.


Line 21: #include "kudu/rpc/messenger.h"
> do we really need all of these includes here, or can some of those move int
Moved some to the .inline.


Line 45: ///
> RpcServiceMgr then?
Refer to comment on line 40. This section is only describing the management of services, but there's more to it than that.


Line 77: /// these pools may be configured by RegisterService().
> what is 'the io system'?
Done


Line 122:   const scoped_refptr<kudu::rpc::ResultTracker> result_tracker() const {
> move to -inline.h
Done


PS2, Line 143: red with all servic
> Do you think this struct is really very necessary? We can do the service re
We would need to retain num_service_threads until we call Init(). But I put Init() into RegisterService() as well, and now StartServices() is really about starting the acceptor threads. So this structure can be removed.


Line 145: 
> how widely does this .h file get included? if it's not super-rare, let's tr
Done


Line 158
> why is this needed?
Removed now.


Line 178
> why not inline?
scoped_refptr is a kind of shared_ptr. This reference needs to be shared with all service objects. I've added a comment.


Line 183
> explain why this needs to be a shared_ptr, ie, who it's shared with.
Done


Line 190
> move to -inline.h
Done


Line 198
> emplace_back instead
Done


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()?
See the method comment. Type P must have a member called thrift_struct. Although this is expected only to apply to ThriftWrapperPB objects, keeping the templatization here means I don't have to declare that class in this header, keeping protobuf out of the way of other consumers of this file.


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


Line 76: 
> where does this come from?
rpc::StatestoreSubscriberIf comes from statestore.service.h, which is auto-generated.


Line 79:   StatestoreSubscriberImpl(RpcMgr* rpc_mgr, StatestoreSubscriber* subscriber)
> why not const&?
Done


Line 89:       status = subscriber_->UpdateState(thrift_request.topic_deltas,
> if this is !ok(), wouldn't thrift_response.status be OK?
thrift_response.status gets set on line 100.


Line 91:           &thrift_response.skipped);
> why isn't !__isset an error?
Good point! I made it required in the thrift struct.


PS2, Line 101:       const ThriftWrapperPB* request, ThriftWrapperPB* respons
> Check the return value?
That's a fair question, but what should we do with it? If we can't write the response object, how can we signal this to caller? (One option is to add an explicit status field to the protobuf, but I'd rather not confuse matters). 

I think it's ok for now to return the response and have the caller fail to deserialize; the error will be caught there.


Line 102:     THeartbeatRequest thrift_request;
> what does this mean if there was a deserialization error?
Deserialization errors are application level. This is used to indicate RPC-level failures.

There is facility in RpcContext to return application errors, but that would change quite a lot of our error handling (e.g. how we assign error codes), and so I consider using that API out of scope for now.


PS2, Line 114:   StatestoreSubscriber* subscriber_;
> Same here.
Same as above.


Line 179:   if (status.ok()) connected_to_statestore_metric_->set_value(true);
> no auto
See comments elsewhere.


Line 180:   if (response.__isset.registration_id) {
> repeating all these template params feels redundant. what does this look li
You have to pass all the parameters at once, which I think is less readable:

  DoRpc(rpc_mgr_, address, request, &response, 100, 3, 500)

a) Who knows what the numeric constants mean?
b) There's no optionality to the arguments (so no neat way to change defaults across RPCs without visiting all call sites)
c) The set of parameters will expand over time to include security parameters, deadlines, feature flags and sidecars. DoRpc() would become really unwieldy.

I eliminated the template parameters (except for the proxy type) by pushing them down to the Execute() method, where they can be inferred.


Line 210:       return Status("--statestore_subscriber_svc_queue_depth must be a positive integer");
> can't it deduce the type param?
type param removed. *Impl is the same naming convention as Kudu. I don't think it's so clear cut that the Impl 'is' the service. 

Fixed the numeric constants.


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

Line 59:     "The number of threads dedicated"
this is clang-format weirdness. I'll fix in the final patch.


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?
No longer [[noreturn]]. Adding the shutdown flag allows this to exit.


Line 73:   StatestoreSubscriber(const std::string& subscriber_id,
> explain param
Done


Line 111: 
> there is no heartbeat service
Done


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

Line 132:       const ThriftWrapperPB* request, ThriftWrapperPB* response, RpcContext* context) {
> why not make this a c'tor param
Ok, changed RegisterService() so it takes a mandatory service implementation, so that now the responsibility for calling the c'tor is with the caller.


Line 366:     subscribers_.erase(subscriber_it);
> if you care about this, shouldn't it be called before the if?
No, otherwise there's a race:

  - ShouldExit() returns false
  - SetExitFlag() is called
  - Try to offer to threadpool
  - Wrong error message printed out.

This is only to get the error message right when the thread pool rejects an update.


Line 734:   OfferUpdate(make_pair(deadline_ms, subscriber->id()), is_heartbeat ?
> why the logic change there? (and why schedule another update if you just un
There was a deadlock possibility that we never hit (https://issues.cloudera.org/browse/IMPALA-2642) that became much more likely because the tests now shut down the statestore and make it possible that OfferUpdate() failed. 

You're right that we should return from line 732.


PS2, Line 772: 
> Should we make these const uints for clarity?
Made them flags.


Line 779: }
> move the body into Start() and remove this function
Start() should not block. Otherwise tests become very awkward to write. Renamed to Join() for clarity.


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

Line 109:   /// Blocks until the exit flag is set. Does not call Start().
> who calls this and does it need to be public?
Yes - anything that starts a statestore and wants to block until it finishes execution.


Line 403:   RpcMgr rpc_mgr_;
> why not inline?
Done


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

Line 19: // common/thrift/StatestoreService.thrift for complementary Thrift data structures.
> why a different namespace?
I have a slight preference for this organisationally (keep generated classes separate), but don't feel strongly so have removed.


Line 21: package impala;
> what for?
Done


Line 22: 
> leave note explaining what .thrift file this is paired up with
Done


Line 23: import "rpc/common.proto";
> these typically end in Msg
KRPC seems to have standardized on *PB, which is more concise. The protobuf style guide is silent on the subject: https://developers.google.com/protocol-buffers/docs/style


http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:

Line 163
> remove commented out code
Leaving in as a reference for what this class used to do. I have a patch to remove all of this, however, which will get squashed into this patch.


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

Line 234:   int64_t count() const { return boost::accumulators::count(acc_); }
> this isn't truly a getter
how not? I don't mind changing it, but I think count(acc_) acts effectively as a getter on acc_.


http://gerrit.cloudera.org:8080/#/c/5720/2/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

Line 147
> leave a note explaining where the service definition that uses the structs 
Done


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 11:

(1 comment)

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

Line 112:   /// 'service_ptr' contains an interface implementation that will handle RPCs.
> May help to comment that 'service_ptr' releases reference to the underlying
I commented that RegisterService() assumes ownership of the underlying object. Let me know if that's not quite what you meant.


-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

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(&thrift_response, response);
Check the return value?


PS2, Line 114:     SerializeThriftToProtoWrapper(&thrift_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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 7:

(1 comment)

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

Line 379:   subscribers_[0]->WaitForFailure(statestore_.get());
> what's done? i was wondering why the line disappeared.
Sorry, moving fast. This was a bug: if the statestore could detect the failure of the beartbeat before it got around to sending an update, the update would never show up. There's no reason for this line - I think it was a bug in the Python version of this test but it never got triggered because I the heartbeat frequency was left at its normal levels. In this test, I decrease the hb frequency significantly (so the test runs faster!), and that triggered this bug.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 7: Code-Review+2

-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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/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/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/client-request-state.cc
M be/src/statestore/CMakeLists.txt
D be/src/statestore/statestore-service-client-wrapper.h
D be/src/statestore/statestore-subscriber-client-wrapper.h
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,733 insertions(+), 1,219 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/15
-- 
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 3:

(28 comments)

Although it might look like a lot has changed in this new patch, there are only two relatively minor big changes:

1. Rewrite test_statestore.py in C++ (see statestore-test.cc) to make sure we don't lose any of that coverage.

2. Remove InProcessStatestore which was unused and causing pain to keep up-to-date.

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


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?
Acceptor threads don't do much (just create a socket and pass it to the reactor threads). This is unlike Thrift where the acceptor thread also did the SASL negotiation, which was expensive and harmed throughput. So really this is double the number of Thrift threads, and each will have less work to do per cnxn.

We expect the number of connections to be much lower with KRPC as well.


PS3, Line 52: 4
> Yeah, this should be plumbed through, will do that.
Done


Line 59:   DCHECK(is_inited()) << "Must call Init() before RegisterService()";
> dcheck that services haven't been started yet, or does that not matter?
Done


Line 62:       new ServicePool(move(scoped_ptr), messenger_->metric_entity(), service_queue_depth);
> why not just pass service_ptr.release()?
I can't quite do that because the gscoped_ptr<T>(T*) c'tor is explicit. I can do:

  new ServicePool(gscoped_ptr<ServiceIf>(service_ptr.release())


PS3, Line 64: service_pool->Init
> good point. if the init call fails, do we abort impalad startup? (i would a
yes indeed (and statestore startup).


PS3, Line 74: int32_t num_acceptor_threads
> so then move the flag to where it's used?
it's used in several places (anywhere that has an RpcMgr might want to use the flag). So I think the DEFINE(..) here, DECLARE(...) in modules that want to use it is the right pattern.


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?
It means that there are 0 services managed by the RpcMgr? Since the RpcMgr also helps get proxies, you don't have to register a service for it to be useful.


PS3, Line 49: CLIENTS
> why define new terminology? isn't this just "proxy" in KuduRPC speak?
I've tried to standardize a bit better. 'clients' and 'servers' are pretty established nouns in this space though.


PS3, Line 52: GetProxy
> so I guess RpcMgr deals with both the server and client side of things?  As
See line 36: "Central manager for all RPC services and clients".

A service is a network-exported set of methods that can be called using the KRPC protocol. Speaking precisely, a service 'definition' is the set of methods (defined in a protobuf), but a service is the instantiation of that service definition on a particular machine. It's a server-side only construct. 

A proxy allows clients to easily call methods on one service, and so they share the same set of method signatures (i.e. they refer to the same service definition). 

Typically a proxy and a service are used from different machines, and KRPC does not check that the remote service has been started before creating a proxy object. Connectivity / service availability checks are performed when the actual RPC method is invoked.


Line 64: /// RpcMgr::RegisterService() before RpcMgr::StartServices() is called.
> does init() go before register()?
Done


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


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),
> how expensive is this?
It's a DNS subsystem call. 

Without the nscd (nameservice caching daemon) running, the mean response time was ~5ms (although the 99.9th percentile was lower - there are larger timeout events that are expensive).

However, nscd brings the mean response time down to about 10us per invocation. We (Cloudera) recommend that users have nscd running for CDH deployments; we should make sure that that is emphasised for Impala as well (it would make a difference no matter if KRPC was used or not).

nscd also seems to reduce the frequency of outliers as well.


Line 41:   proxy->reset(new P(messenger_, addresses[0]));
> dcheck that addresses isn't empty?
Done


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

PS3, Line 36: Clients
> over in rpc-mgr.h, you seemed to use "client" as a synonym for proxy, but w
I've reworded rpc-mgr.h to standardize on Proxy. I'll write 'callers' here to avoid ambiguity.

'A single RPC instance' is a bit confusing, I've reworded. But it's intended to mean a single invocation of a remote method. The point at which you consider a method 'invoked' seems to be causing some confusion: here I mean to be the point at which application logic actually gets called on the server side.


PS3, Line 48: Rpc
> I was confused about this class at first since the name sounds like it's th
Retry semantics are often application-specific. For example, Kudu have different retry requirements than us - for example, they want to fail over to different replicas. (They do have a retry object, but it's very Kudu-specific).

I don't think it would help to call this a Wrapper class. From the perspective of the caller, this class presents the abstraction of an RPC: the caller invokes a method (via Execute()), and gets a response.


Line 51:   /// 'RESP' (must both be protobuf).
> unclear what req and resp refer to
Outdated comment, done.


Line 58:   Rpc(const TNetworkAddress& remote, RpcMgr* mgr) : remote_(remote), mgr_(mgr) {}
> does this need to be public?
Done


Line 68:   Rpc& SetMaxAttempts(int32_t max_attempts) {
> does this need to be specific (int32_t as opposed to int)?
It doesn't have to be, but I thought in general we tried to be explicit. Can revert it to an int if preferred.


PS3, Line 90: func
> point out in what way F is constrained
Done


Line 91:     if (max_rpc_attempts_ <= 1) {
> why not dcheck in set..()? there's no reason this should be a runtime error
Typo - should be <, not <=.

Re: DCHECK vs runtime error - I can see these parameters being controlled by flags (e.g the timeout for the data stream sender), so they will need to have runtime checks.


Line 112:       if (!controller.status().IsRemoteError()) break;
> comment on significance of remote error. looks like you're getting a non-ok
Rewritten this to make it a bit clearer. I've introduced IsRetryableError() (which is also needed in a subsequent patch) to abstract the logic to decide if a retry is needed.


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.
I think we should just set a default. It's now a *really* bad idea to have RPCs without timeouts, because of the limited set of service threads.


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
Done


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 
Yes - this will go away.


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 
How about just 'service'? ServiceImpl is a bit clunky, and there aren't going to be multiple implementations of the *If class.


Line 207:       return Status("--statestore_subscriber_num_svc_threads must be at least 2");
> why not just bump it up to 2?
I prefer to avoid surprise by changing flags if they're set wrong. I suspect no-one's going to set this lower.


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?
Yes - when the ImpalaInternalService is ported to KRPC, this should go away.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

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

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <he...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................


Patch Set 7:

(1 comment)

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

Line 55: template <typename P>
> p is only used in Execute(). why not add p as a template parameter to that 
I think if we do that, we'd have to list all the template parameters for Execute(); i.e. if it can't infer one of them we have to list all of them.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-{4670,4672,4784}: Add RpcMgr and port Statestore services to KRPC
......................................................................

IMPALA-{4670,4672,4784}: 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. The new Rpc
class helps clients build rpc invocation objects, and use them to call
remote methods.

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.
 * Remove InProcessStatestore that was only used for statestore-test and
   mini-impala-cluster. As far as we know, mini-impala-cluster is unused
   by anyone. (IMPALA-4784)

TESTING:
 * Impala's test suite passes.
 * statestore-test has been rewritten to use new statestore code. It
   includes all test cases from the now-removed Python test_statestore.py
 * rpc-mgr-test unit-tests the RpcMgr and Rpc classes.

TODO:
 * Enable SSL and Kerberos support (already in Kudu's imported
   libraries)

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
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/testutil/in-process-servers.h
D be/src/testutil/mini-impala-cluster.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 be/src/util/counting-barrier.h
M bin/start-impala-cluster.py
M bin/start-impalad.sh
M common/thrift/CMakeLists.txt
M common/thrift/StatestoreService.thrift
D tests/statestore/test_statestore.py
53 files changed, 1,738 insertions(+), 1,083 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/5720/5
-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>