[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has abandoned this change.

Change subject: Allow to release an rpc transfer's data
..


Abandoned

turns out all that's needed to do this is not to reuse the KuduScanBatch. The 
data will live as long as the batch lives

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to release an rpc transfer's data

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6592/8/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

Line 327:   Status ReleaseTransferData(uint8_t** released_data);
why not make this a unique_ptr* so that it is more explicitly 
transferring ownership, and ensures that the caller uses the right delete[] 
variant?


http://gerrit.cloudera.org:8080/#/c/6592/8/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

Line 200:   // Deleting 'response_buffer' must be done through "delete []".
same, using unique_ptr would ensure this


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to release an rpc transfer's data
..

Allow to release an rpc transfer's data

This adds a way to release the current response's buffer to a caller,
enabling external lifetime management of this data.
Users of this api _MUST_ have obtained pointers to the individual
sidecars before obtaining a pointer to the raw data block through the
new API.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 73 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/6592/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6592
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6592/6/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

Line 327:   Status ReleaseTransferData(uint8_t** transfer_data);
> warning: function 'kudu::rpc::CallResponse::ReleaseTransferData' has a defi
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to release an rpc transfer's data
..

Allow to release an rpc transfer's data

This adds a way to release the current response's buffer to a caller,
enabling external lifetime management of this data.
Users of this api _MUST_ have obtained pointers to the individual
sidecars before obtaining a pointer to the raw data block through the
new API.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 73 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/6592/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6592
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to release an rpc transfer's data
..

Allow to release an rpc transfer's data

This adds a way to release the current response's buffer to a caller,
enabling external lifetime management of this data.
Users of this api _MUST_ have obtained pointers to the individual
sidecars before obtaining a pointer to the raw data block through the
new API.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 73 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/6592/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6592
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6592/5//COMMIT_MSG
Commit Message:

PS5, Line 10: obtain data for decoding
> what do you mean by that?
Done


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS5, Line 324: should
> should be obtained? or MUST be obtained? (i.e after releasing transfer data
Done


PS5, Line 323:   // Returns a pointer to the transfer data, including the 
header and all
 :   // sidecars. Pointers to individual sidecars should be 
obtained before
 :   // calling this method. That is, this method is not meant to 
return
 :   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
> maybe move this doc to RpcController so it's user-facing, and just leave a 
Done


PS5, Line 326:   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
 :  
> if it only is used for ownership, then why do we need to get a Slice instea
Done


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

PS5, Line 192:   // Resets this controller, but releases the transfer data 
before.
 :   Status ReleaseTransferDataAndReset(Slice* release_data);
> "transfer data" here is too implementation-centric.
Done


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

PS5, Line 87:   // NOTE: buf_.release() doesn't memcpy if the transfer has 
completed, because
:   // buf_ must have been mandatorily resized from the initial 
capacity of 4 bytes
:   // to receive the whole message.
:  
> not sure I follow. The initial capacity of faststring is 32 bytes, so it's 
removed this comment, the worse that can happen is that the data is memcpy'd in 
the that case but I don't think that matters.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to release an rpc transfer's data

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6592/5//COMMIT_MSG
Commit Message:

PS5, Line 10: obtain data for decoding
what do you mean by that?


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS5, Line 324: should
should be obtained? or MUST be obtained? (i.e after releasing transfer data, is 
it legal to call other methods on this instance?)


PS5, Line 323:   // Returns a pointer to the transfer data, including the 
header and all
 :   // sidecars. Pointers to individual sidecars should be 
obtained before
 :   // calling this method. That is, this method is not meant to 
return
 :   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
maybe move this doc to RpcController so it's user-facing, and just leave a 
breadcrumb here (like above)


PS5, Line 326:   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
 :  
if it only is used for ownership, then why do we need to get a Slice instead of 
just the pointer to the start? i.e we're only expecting the user to later 
'delete' this, right?


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

PS5, Line 192:   // Resets this controller, but releases the transfer data 
before.
 :   Status ReleaseTransferDataAndReset(Slice* release_data);
"transfer data" here is too implementation-centric.

I think the name should probably be like ReleaseResponseBufferAndReset or 
somesuch.


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

PS5, Line 87:   // NOTE: buf_.release() doesn't memcpy if the transfer has 
completed, because
:   // buf_ must have been mandatorily resized from the initial 
capacity of 4 bytes
:   // to receive the whole message.
:  
not sure I follow. The initial capacity of faststring is 32 bytes, so it's 
still possible it as to alloc and memcpy, but we don't really care, because the 
case of a <32b response is rare?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to release an rpc transfer's data
..

Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership and to allow for optimizations where no
memcpy's are done on the client side.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 72 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/6592/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6592
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot