[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has abandoned this change. Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Hello Marcel Kornacker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6473 to look at the new patch set (#8). Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. IMPALA-4889: Use client sidecars for Thrift RPCs This patch changes the way Thrift structures are serialized with KRPC. Instead of serializing them to a byte stream, and then writing that stream to a Protobuf object which is serialized again en route to the wire, the Thrift objects are serialized to byte streams which are then directly written to the wire as a 'sidecar'. This saves a copy and serialization step both on the sender and receiver sides of the RPC. Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 --- M be/src/rpc/common.proto M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc.h M be/src/rpc/thrift-util.h M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc 7 files changed, 135 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/6473/8 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Hello Marcel Kornacker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6473 to look at the new patch set (#4). Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. IMPALA-4889: Use client sidecars for Thrift RPCs This patch changes the way Thrift structures are serialized with KRPC. Instead of serializing them to a byte stream, and then writing that stream to a Protobuf object which is serialized again en route to the wire, the Thrift objects are serialized to byte streams which are then directly written to the wire as a 'sidecar'. This saves a copy and serialization step both on the sender and receiver sides of the RPC. Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 --- M be/src/rpc/common.proto M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc.h M be/src/rpc/thrift-util.h M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc 7 files changed, 135 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/6473/4 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6473/1/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
Line 169: int idx = -1;
> what's undefined about it?
Hm, looks like compilers won't warn in this case (I thought they'd be more
conservative), but turns out:
void t(int* p) { }
int foo() {
int i;
t(&i);
if (i) { }
}
doesn't give a warning even though i is uninitialized (whereas if you omit the
call to t(), it does).
Long story short, I removed the initialization.
Line 179: RETURN_IF_ERROR(DeserializeThriftMsg(sidecar.data(), &len, true,
resp));
> how long is sidecar.data() valid?
As long as the controller_ object is valid, so as long as the Rpc is valid
(which is usually not very long after this method returns).
--
To view, visit http://gerrit.cloudera.org:8080/6473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Marcel Kornacker
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has uploaded a new patch set (#3). Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. IMPALA-4889: Use client sidecars for Thrift RPCs This patch changes the way Thrift structures are serialized with KRPC. Instead of serializing them to a byte stream, and then writing that stream to a Protobuf object which is serialized again en route to the wire, the Thrift objects are serialized to byte streams which are then directly written to the wire as a 'sidecar'. This saves a copy and serialization step both on the sender and receiver sides of the RPC. Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 --- M be/src/rpc/common.proto M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc.h M be/src/rpc/thrift-util.h M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc 7 files changed, 125 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/6473/3 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/6473/1/be/src/rpc/rpc.h File be/src/rpc/rpc.h: Line 169: int idx = -1; > I think it's technically undefined behavior not to initialize it here becau what's undefined about it? to me this looks like the function has undefined behavior in the idx==-1 case, from the perspective of the caller, which i'd prefer to avoid. Line 179: RETURN_IF_ERROR(DeserializeThriftMsg(sidecar.data(), &len, true, resp)); how long is sidecar.data() valid? -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6473/1/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
Line 101: // RPC invocation using Execute().
> why synchronous?
This patch doesn't include async RPCs. I'll remove the references. (They're in
the next, big patch).
Just to explain this case: an async RPC may complete and call its callback
after its Rpc object has been destroyed. In that case, the RpcController is
passed directly to the async completion callback.
Line 102: Status GetInboundSidecar(int idx, kudu::Slice* sidecar) {
> 'inbound' sounds a bit misleading, my initial response was 'you mean server
Changed to GetResponseSidecar().
Line 169: int idx = -1;
> that shouldn't be necessary.
I think it's technically undefined behavior not to initialize it here because
the compiler can't tell that it gets initialized in AddSidecar().
Line 170: AddSidecar(kudu::Slice(serialized), &idx);
> looks like this is making a copy, bummer. is there some way to avoid that?
It's only copying the slice (which is basically a ptr + len pair). See
documentation for AddSidecar() - the memory backing the slice is not copied.
Line 176: RETURN_IF_ERROR(GetInboundSidecar(response_proto.sidecar_idx(),
&sidecar));
> do we not want to reserve this mechanism for large pieces of data?
I think it works equally well for small and large, and saves the extra copy in
both cases.
Line 183: Rpc(const Rpc& other) {
> comment why needed
Removed - needed for the next patch.
Line 207: // Rpc controller storage. Used only for synchronous RPCs so that
the caller can access
> how do we do async rpcs again?
They're in the next patch - references to them here are the result of
re-ordering the patches.
--
To view, visit http://gerrit.cloudera.org:8080/6473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Marcel Kornacker
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has uploaded a new patch set (#2). Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. IMPALA-4889: Use client sidecars for Thrift RPCs This patch changes the way Thrift structures are serialized with KRPC. Instead of serializing them to a byte stream, and then writing that stream to a Protobuf object which is serialized again en route to the wire, the Thrift objects are serialized to byte streams which are then directly written to the wire as a 'sidecar'. This saves a copy and serialization step both on the sender and receiver sides of the RPC. Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 --- M be/src/rpc/common.proto M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc.h M be/src/rpc/thrift-util.h M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc 7 files changed, 125 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/6473/2 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Marcel Kornacker
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6473/1/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:
Line 101: // RPC invocation using Execute().
why synchronous?
Line 102: Status GetInboundSidecar(int idx, kudu::Slice* sidecar) {
'inbound' sounds a bit misleading, my initial response was 'you mean
server-side?' instead of 'this is a return value'.
Line 169: int idx = -1;
that shouldn't be necessary.
Line 170: AddSidecar(kudu::Slice(serialized), &idx);
looks like this is making a copy, bummer. is there some way to avoid that?
Line 176: RETURN_IF_ERROR(GetInboundSidecar(response_proto.sidecar_idx(),
&sidecar));
do we not want to reserve this mechanism for large pieces of data?
Line 183: Rpc(const Rpc& other) {
comment why needed
Line 207: // Rpc controller storage. Used only for synchronous RPCs so that
the caller can access
how do we do async rpcs again?
--
To view, visit http://gerrit.cloudera.org:8080/6473
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson
Gerrit-Reviewer: Marcel Kornacker
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4889: Use client sidecars for Thrift RPCs
Henry Robinson has uploaded a new change for review. http://gerrit.cloudera.org:8080/6473 Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs .. IMPALA-4889: Use client sidecars for Thrift RPCs This patch changes the way Thrift structures are serialized with KRPC. Instead of serializing them to a byte stream, and then writing that stream to a Protobuf object which is serialized again en route to the wire, the Thrift objects are serialized to byte streams which are then directly written to the wire as a 'sidecar'. This saves a copy and serialization step both on the sender and receiver sides of the RPC. Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 --- M be/src/rpc/common.proto M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc.h M be/src/rpc/thrift-util.h M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc 7 files changed, 135 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/6473/1 -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson
