[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has submitted this change and it was merged. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Reviewed-on: http://gerrit.cloudera.org:8080/7599 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 532 insertions(+), 159 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 5: The verification failure may be infra related. Not sure if I have the permission to request a re-run. 13:52:46 F0817 20:52:08.278630 31810 master_main.cc:68] Check failed: _s.ok() Bad status: Network error: error binding socket to 0.0.0.0:40300: Address already in use (error 98) 13:52:46 *** Check failure stack trace: *** -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7599 to look at the new patch set (#5). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 532 insertions(+), 159 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/5 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 254: // sidecars attached to the call as it may result in use-after-free of the sidecars. > per comment in header, I think it's worth explaining why this is reasonable Added some explanation in the header. Refer to it here. PS3, Line 273: outbound transfer for this call so references to the sidecars : // can be relinquished. > I think this is no longer necessary to be added since the explanation of th Done http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS3, Line 304: e.g. older version of Kudu server), it's fatal for the > it's not clear what behavior this is implying. If it will FATAL, I think we Done http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: Line 223: return sidecar_byte_size_ > 0; > this isn't quite accurate to what the description says. It is possible to h Right. Comments updated. Renamed function to HasNonEmptySidecars(). -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7599 to look at the new patch set (#4). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 532 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/4 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 254: // attached to the call or it may result in use-after-free of the sidecars. per comment in header, I think it's worth explaining why this is reasonable PS3, Line 273: We assume that the remote supports footer or the call : // doesn't have any sidecars. I think this is no longer necessary to be added since the explanation of this assumption is in the CancelOutboundTransfer function itself. http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS3, Line 304: the assumption is that the call doesn't have any sidecar. it's not clear what behavior this is implying. If it will FATAL, I think we should be explicit here (and perhaps reproduce the reasoning from our gerrit discussion that we didn't start using outbound sidecars until the same version when we introduced footers, therefore it's not possible to have a call with sidecars but no footer support) http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: Line 223: return sidecar_byte_size_ > 0; this isn't quite accurate to what the description says. It is possible to have an empty sidecar (I think we actually do this in the case of an empty scan result, for example) -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7599 to look at the new patch set (#3). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 526 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/3 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change.
Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or
timeout
..
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 251: } else {
> yea I think that is actually reasonable. I'd do CHECK though, since this is
Done
PS2, Line 268:
: // The timeout timer is
> per above, this is still best-effort in the case that you are talking to a
Comments updated.
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS2, Line 199: etes.
> Comment rephrased.
Actually, comment removed.
PS2, Line 201: the negotiation thread
> ah, I see... negotiation_complete_ is set by the reactor thread itself wher
Yes, added a small note about this in the code.
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
PS2, Line 352: // behavior is a lot more efficient if memory is freed from
the same thread
: // which allocated it -- this le
> looking at the implementation of faststring::release() I think these calls
Done
--
To view, visit http://gerrit.cloudera.org:8080/7599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); > I just did a grep in the Kudu code. There doesn't seem to be any use of Rpc yea I think that is actually reasonable. I'd do CHECK though, since this is rare so we don't care about perf, and I'd rather have an obvious crash than a weird memory corruption -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); > Thanks for summarizing. I just did a grep in the Kudu code. There doesn't seem to be any use of RpcController::AddOutboundSidecar(). I suppose if Kudu starts using client sidecar for some new functionality, it needs to update the Kudu server code to make use of the sidecars too so mixed version doesn't really apply in this case. May be it's safe to just add a DCHECK to verify that the sidecars' bytes are zero if remote doesn't support footers or will it cause any confusion ? -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change.
Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or
timeout
..
Patch Set 2:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 251: car->transfer->Abort(status);
> hm... to summarize the possibilities here:
Thanks for summarizing.
#3 seems attractive but I am a bit worried about the arbitrarily large sidecars
which we have to buffer. It may end up pushing Impalad over the process memory
limit and getting it OOM killed.
For all practical purposes in Impala, we envision the first version of Impala
using KRPC will have both server and client supporting footers. #2 is not ideal
for mixed cluster usage with client sidecars but arguably it's not a regression
either. I don't know how common the usage of client sidecars in Kudu today is.
If Impala is the only use case, may be it's safer to stick with #2 for now.
Yet another idea is to fabricate certain byte sequence which will always fail
to deserialize in the remote server but I don't think it will work once the
request PB has been sent so I won't bet on it.
Please let me know what you think.
PS2, Line 703:if (!transfer->TransferStarted()) {
: if (transfer->is_for_outbound_call()) {
: if (!StartCallTransfer(transfer)) {
> these can all be collapsed into a single boolean expression now, right?
Done
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS2, Line 199: takes
> typo: take
Comment rephrased.
PS2, Line 199: Callers should takes this into account.
> not quite sure what this sentence is indicating. Don't we assume that calle
Rephrased the comment. It intends to point out that this function will return
false before negotiation completes so callers (in particular reactor threads)
need to handle it properly by doing the "right thing" (whatever that means for
the caller).
PS2, Line 284: sending it for
: // the first time
> I think better to say "beginning to send it" or "beginning transmission" or
Done
Line 288: // If the checks pass, the outbound call will be set to 'SENDING'
state.
> if the checks don't pass, is this responsible for 'finishing' the call? eg
Comments added.
PS2, Line 289: Append
> nit: "Appends"
Done
PS2, Line 303: if the transfer is for an outbound call.
> not quite clear here which part of the sentence the "if ..." applies to. If
Thanks for the suggestion. Done.
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
Line 29: #include "kudu/rpc/outbound_call.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 30: #include "kudu/rpc/connection.h"
> fix sort order
Done
PS2, Line 260: shutted
> nit: "shut"
Done
Line 266: // fall-through if remote supports parsing footer.
> we have a special macro here 'FALLTHROUGH_INTENDED' which turns into someth
Done
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:
PS2, Line 159: send_footer
> is this out of date? I don't see this param
Done
Line 171: // to indicate that the call has been aborted.
> worth indicating whether this call requires that AppendFooter() has previou
Done
PS2, Line 210: IsBeingSent
> Lol. I considered using IsOnTransferQueue() at some point.
Actually, I now recall why I ruled against using that name as it can easily
mislead readers into believing that the call is in ON_OUTBOUND_QUEUE state.
Renamed to IsInTransmission() ?
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
PS2, Line 274: Note that the footer is designed to be modified
: // after the initial serialization and it will be
re-serialized after modification.
: // To avoid unexpected change in the total message size, keep
to using fixed sized
: // fields only in the footer.
> move this comment to be below the 'aborted' field, since it documents any n
Done
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/serialization.h
File src/kudu/rpc/serialization.h:
Line 78: void IncrementMsgLength(size_t inc_len,
> warning: function 'kudu::rpc::serialization::IncrementMsgLength' has a defi
Done
Line 78: void IncrementMsgLength(size_t inc_len,
> please fix this nit
Done
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:
Line 78: /* Initialize the dummy buffer with a known pattern for easier
debugging. */
> style: // C++ style comment
Done
Line 190: ++n_payload_slices_;
> how about a post-increment on the prior line?
Done
--
To view, visit http://gerrit.cloudera.org:8080/7599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-M
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); > Good point. We could also reach this point because negotiation has not comp hm... to summarize the possibilities here: - on caller-requested cancellation: -- if pre-transmission or post-transmission, immediately call callback with Aborted case -- if mid-transmission: --- if footers are negotiated, do the footer magic and immediately call callback --- else: just treat as a no-op (this case isn't really expected so it's OK to be best-effort) - on timeout: -- if pre-transmission or post-transmission, call callback -- if mid-transmission and sidecars are not in use, or sidecars in use but footers are supported, abort and call callback -- if mid-transmission and sidecars are in use, but footers are not enabled, then we are in the sticky situation described above. The choices in this sticky situation are: 1) "delay the timeout" -- as you suggested above, set some flag, and then call the timeout callback when the transmission is complete. Unfortunately that could be arbitrarily long into the future, and Kudu at least would not be very happy with that. 2) "call the timeout anyway" -- which risks use-after-free of the sidecar in the Impala use case (i.e this is today's buggy behavior) 3) "copy the remaining sidecar and call immediately" - what if we actually just allocated a new buffer, copied the remaining data into it, and relocated the slices to point to it instead of the original data? We have some untracked memory which is ugly, but we avoid the use-after-free, and we don't expect this situation to really happen in practice since it only would happen in a mixed-version cluster which is also using client sidecars. Any thoughts on choice #3? hm... one thing I am a little afraid of in the timeout case is having a regression on timeout "timeliness" in the existing Kudu use case. If we set a 1 second timeout, we really expect it to return in one second even if we are stuck sending. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 201: negotiation_complete_ && > I need this to avoid a TSAN warning about concurrent access to remote_featu ah, I see... negotiation_complete_ is set by the reactor thread itself whereas remote_features_ is directly mutated by the negotiator. Good job TSAN! this would have been a nasty bug. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: Line 223: // the transfer if only the footer is left. > Actually, in this case, we will still invoke OutboundCall::Cancel() right a ah, I see, that's right. -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: (4 comments) Thanks again for taking a look. Replies to some questions below before addressing the rest of the comments. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); > what about in the case of talking to an old server which doesn't support fo Good point. We could also reach this point because negotiation has not completed yet. It seems always safe to always call Abort() if negotiation hasn't completed yet as we shouldn't have started sending anything yet. Once negotiation has completed and the remote doesn't support footer, yes, then this should be a no-op for cancellation. It slightly more complicated for the time-out case if the remote doesn't support footer. We cannot invoke OutboundCall::SetTimedOut() right away in HandleOutboundCallTimeout() below if the call is mid-transmission or we will hit the original bug again. We probably need to leave a status or bool indicating that the call has timed out and in OutboundCall::SetSent(), we need to invoke the SetTimedOut() instead of waiting for the response. Please let me know if you think the above proposal is too complicated. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 201: negotiation_complete_ && > is negotiation_complete_ necessary? before negotiation is complete, isn't r I need this to avoid a TSAN warning about concurrent access to remote_features_ between the negotiation thread and the reactor thread. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS2, Line 210: IsBeingSent > hrm, I actually am going to disagree with my previous comment and still thi Lol. I considered using IsOnTransferQueue() at some point. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: Line 223: // the transfer if only the footer is left. > could this last bit result in a "hung" cancellation in the case that the se Actually, in this case, we will still invoke OutboundCall::Cancel() right away in ReactorThread::CancelOutboundCall() after calling into this function via Connection::CancelOutboundCall(). It's true that the OutboundCall object may still hang around for a while because the TransferCallback still has a reference to it. Please let me know if I may have misunderstood your comment. -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or
timeout
..
Patch Set 2:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 251: car->transfer->Abort(status);
what about in the case of talking to an old server which doesn't support
footer, but still mid-transmission? in that case don't we want to no-op this
function call? here it seems like you are still aborting it, which would result
in calling the callback while the sidecar is still referenced, and thus hitting
the original bug again.
PS2, Line 268: // Cancel or abort any outbound transfer for this call so
references to the sidecars
: // can be relinquished.
per above, this is still best-effort in the case that you are talking to a
remote which doesn't support this feature.
PS2, Line 703:if (!transfer->TransferStarted()) {
: if (transfer->is_for_outbound_call()) {
: if (!StartCallTransfer(transfer)) {
these can all be collapsed into a single boolean expression now, right?
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS2, Line 199: takes
typo: take
PS2, Line 199: Callers should takes this into account.
not quite sure what this sentence is indicating. Don't we assume that callers
should always take everything documented into account? :)
PS2, Line 201: negotiation_complete_ &&
is negotiation_complete_ necessary? before negotiation is complete, isn't
remote_features_ guaranteed to be an empty set, in which case it will already
be false?
PS2, Line 284: sending it for
: // the first time
I think better to say "beginning to send it" or "beginning transmission" or
somesuch
Line 288: // If the checks pass, the outbound call will be set to 'SENDING'
state.
if the checks don't pass, is this responsible for 'finishing' the call? eg if
it doesn't support the feature flags, will this take care of calling the
callback?
PS2, Line 289: Append
nit: "Appends"
PS2, Line 303: if the transfer is for an outbound call.
not quite clear here which part of the sentence the "if ..." applies to. If I
understand correctly, maybe this should say:
// Remove the front transfer from the outbound transfers queue.
// If the transfer is associated with an outbound call, clears the transfer
reference from the associated CallAwaitingResponse.
or something like that, to be more clear that it _always_ removes the front,
and then the reference removal is in addition to that, in certain circumstances.
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
Line 30: #include "kudu/rpc/connection.h"
fix sort order
PS2, Line 260: shutted
nit: "shut"
Line 266: // fall-through if remote supports parsing footer.
we have a special macro here 'FALLTHROUGH_INTENDED' which turns into something
magic in clang (see gutil/macros.h)
PS2, Line 352: delete [] header_buf_.release();
: delete [] footer_buf_.release();
looking at the implementation of faststring::release() I think these calls are
actually not a great idea, since, if the data is less than 32 bytes, it will be
stored inline in the faststring object, and then these 'release' calls actually
cause an allocation and copy into a heap buffer.
For the header, I think it's typically >32 bytes so this is beneficial, but for
the footer it's almost certainly not.
I think it's better to change both of these to call clear() followed by
shrink_to_fit(), which will free the memory only if it's a separate allocation.
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:
PS2, Line 159: send_footer
is this out of date? I don't see this param
Line 171: // to indicate that the call has been aborted.
worth indicating whether this call requires that AppendFooter() has previously
been called. What happens if no AppendFooter() was called? would it CHECK fail?
(probably should?)
PS2, Line 210: IsBeingSent
hrm, I actually am going to disagree with my previous comment and still think
this isn't a great name, since it includes the "scheduled but hasn't started
sending". Maybe "IsQueuedForTransfer" or 'IsOnTransferQueue' or something?
http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
PS2, Line 274: Note that the footer is designed to be modified
: // after the initial serialization and it will be
re-serialized after modification.
: // To avoid unexpected change in the total message size, keep
to using fixed sized
: // fields only in the footer.
move this comment to be below the 'aborted' field, since it documents any new
fie
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change.
Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or
timeout
..
Patch Set 2:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/7599/1//COMMIT_MSG
Commit Message:
PS1, Line 9: call times out
> worth specifying "times out after transmission of the request has begun but
Done
Line 17: of the sidecars with the RPC layer. This has caused some life cycle
> mind linking to the Impala JIRA where it was discovered this lifecycle was
Done
PS1, Line 20: fixes the
> not in-flight but mid-transmission. We still don't support a preemptive "ab
Done
PS1, Line 22: . It c
> nit: again I think "mid-transmission" is clearer since we usually call an R
Done
http://gerrit.cloudera.org:8080/#/c/7599/1/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:
PS1, Line 274: | RPC Footer protobuf length (variable encoding) | --- Exists
only in client requests.
: ++
: | RPC Footer protobuf| --- Exists
only in client requests.
: +
> This being a variable-length protobuf is a little bit risky, in the sense t
Good point. Doesn't the RPC header have the same problem but I suppose there is
currently no code which changes the header after serializing it ?
I will add a documentation to state that the footer must only have fixed size
fields and DCHECK the modified footer doesn't change the total size.
PS1, Line 366: cancelled mid-transmission.
> should clarify "mid-tranmission" here
Done
Line 387:"\x0c" Request parameter varint: 12 bytes
> can you add some note here that says this capture does _not_ include the fo
Added footer to this message. Also updated the output for RequestHeader and the
decoding command.
http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 240: DCHECK(reactor_thread_->IsCurrentThread());
> can you DCHECK that it's the reactor thread here just for documentation val
Done
Line 448: DCHECK_EQ(call_from_map, call_.get());
> warning: parameter 'status' is unused [misc-unused-parameters]
Done
Line 574:
> I think this should probably be VLOG(2) and need a lot more context to be u
Changed the log level to 2.
I added some more details in Status::Aborted() returned from
InboundCall::ParseFrom(...).
PS1, Line 649: const set& required_features =
car->call->required_rpc_features();
: if (!includes(remote_features_.begin(), remote_features_.end(),
: required_features.begin(),
required_features.end())) {
: Status s = Status::NotSupported("server does not support the
required RPC features");
: transfer->Abort(s);
: car->call->SetFailed(s, Phase::REMOTE_CALL);
: // Test cancellation when 'call_' is in 'FINISHED_ERROR'
state.
: MaybeInjectCancellation(car->call);
: car->call.reset();
: return false;
: }
:
: // The transfer is ready to be sent. Append a footer if the
remote system supports it.
: // It has to be done here because remote features cannot be
determined until negotiation
: // completes. We know for sure that negotiation has completed
when we reach this point.
: if (RemoteSupportsFooter()) {
: transfer->AppendFooter(car->call);
: }
: car->call->SetSending();
: // Test cancellation when 'call_' is in 'SENDING' state.
: MaybeInjectCancellation(car->call);
: return true;
: }
:
: void Connection::OutboundQueuePopFront() {
: OutboundTransfer *transfer = &(outbound_transfers_.front());
: // Remove all references to 'transfer' before deleting it.
: if (transfer->is_for_outbound_call()) {
: CallAwaitingResponse* car = FindOrDie(awaiting_response_,
transfer->call_id());
: car->tr
> do you think we could refactor out these ~30 lines into a new function? Thi
Good idea. The code looks cleaner after the refactoring.
http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
Line 198: // Returns true if the remote end has feature flag
"REQUEST_FOOTERS". Always returns false
> I think a better name is something like 'RemoteSupportsFooters' or somethin
Done
Line 199: // before negotiation completes. Callers should takes this into
account.
> use ContainsKey from map-util.h
Done. Also added negotiation_complete_ &&
http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:
PS1, Line 228:
: RequestFooterPB footer_;
> no n
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7599 to look at the new patch set (#2). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out after transmission of the request has begun but not finished, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This has caused some life cycle issues with row batches in Impala before (e.g. IMPALA-5093). This change fixes the problem above by modifying the RPC protocol to allow a mid-transmission RPC call to be cancelled. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the outbound call was cancelled after it has started but before it finished. The server will inspect this flag and ignore inbound calls with this flag set. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the server always ignores cancelled call requests, it's okay to send random bytes. To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS is introduced in this change. During connection negotiation, the client will always include this flag in its feature set. A server which supports parsing footer will include REQUEST_FOOTERS in its feature set if it sees the client also supports it. A client will only send the footer if the remote server has the RPC feature flag REQUEST_FOOTERS. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/rtest.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 19 files changed, 481 insertions(+), 152 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/2 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or
timeout
..
Patch Set 1:
(28 comments)
http://gerrit.cloudera.org:8080/#/c/7599/1//COMMIT_MSG
Commit Message:
PS1, Line 9: call times out
worth specifying "times out after transmission of the request has begun but not
finished" or something
Line 17: the caller's point of view for resource management.
mind linking to the Impala JIRA where it was discovered this lifecycle was
causing trouble?
PS1, Line 20: in-flight
not in-flight but mid-transmission. We still don't support a preemptive "abort"
command for an RPC which has already been fully transmitted, right?
PS1, Line 22: flight
nit: again I think "mid-transmission" is clearer since we usually call an RPC
"in-flight" while it has been fully sent and awaiting a response
http://gerrit.cloudera.org:8080/#/c/7599/1/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:
PS1, Line 274: | RPC Footer protobuf length (variable encoding) | --- Exists
only in client requests.
: ++
: | RPC Footer protobuf| --- Exists
only in client requests.
: +
This being a variable-length protobuf is a little bit risky, in the sense that
you are modifying the value here after you've started transmitting the message
(i.e. after computing 'total message length'). So, you're requiring that any
value modification you do doesn't change the serialized length of the message?
It seems like a safe bet that booleans are always going to be one byte, but we
should make sure we have a clear note somewhere about this -- perhaps in the
protobuf definition something that says "any fields which are computed during
the process of sending the body of the message must be fixed length protobuf
types"?
PS1, Line 366: cancelled before it completes
should clarify "mid-tranmission" here
Line 387: ```
can you add some note here that says this capture does _not_ include the footer?
http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 240: DCHECK(car->call);
can you DCHECK that it's the reactor thread here just for documentation value?
Line 574: VLOG(1) << "inbound transfer aborted " << s.ToString();
I think this should probably be VLOG(2) and need a lot more context to be
useful (or just remove)
PS1, Line 649: CallAwaitingResponse* car =
FindOrDie(awaiting_response_, transfer->call_id());
: if (!car->call) {
: // If the call has already timed out or has already
been cancelled, the 'call'
: // field would be set to NULL. In that case, don't
bother sending it.
: DCHECK(transfer->TransferAborted());
: send_buffer = false;
: } else {
: DCHECK(!transfer->TransferAborted());
: // If this is the start of the transfer, then check if
the server has the
: // required RPC flags. We have to wait until just
before the transfer in
: // order to ensure that the negotiation has taken
place, so that the flags
: // are available.
: const set& required_features =
: car->call->required_rpc_features();
: if (!includes(remote_features_.begin(),
remote_features_.end(),
: required_features.begin(),
required_features.end())) {
: Status s =
: Status::NotSupported("server does not support
the required RPC features");
: transfer->Abort(s);
: car->call->SetFailed(s, negotiation_complete_ ?
: Phase::REMOTE_CALL :
Phase::CONNECTION_NEGOTIATION);
: // Test cancellation when 'call_' is in
'FINISHED_ERROR' state.
: MaybeInjectCancellation(car->call);
: car->call.reset();
: send_buffer = false;
: } else {
: car->call->SetSending();
: // Test cancellation when 'call_' is in 'SENDING'
state.
: MaybeInjectCancellation(car->call);
: }
do you think we could refactor out these ~30 lines into a new function? This
flow inside this while loop is getting a bit hard to follow with the 'bool
send_buffer' thing, and I think I could parse it a bit better if this were all
something like 'PrepareToSendCall(car)' returning an enum or somesuch
http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.h
File src/k
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has uploaded a new change for review. http://gerrit.cloudera.org:8080/7599 Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This is not always desirable from the caller's point of view for resource management. This change fixes the problem above by modifying the RPC protocol to allow an in-flight RPC call to be aborted. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the request was cancelled mid-flight and the inbound call should be ignored altogether. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the aborted flag is set in the footer, the inbound call will be ignored anyway so it's okay to send random bytes. To accommodate the new footer, a new RPC feature flag HAS_FOOTER is also introduced in this change. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 15 files changed, 328 insertions(+), 99 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/1 -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has uploaded a new patch set (#2). Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout Previously, when an outbound call times out, the RPC layer will continue to send the entire payload (including any sidecars) to the remote destination until the promised number of bytes specified in the header are sent. This is problematic for the users of the RPC layer as there is no well defined point in which the sidecars are no longer referenced by the RPC layer. The only model in which this would work is for the caller to either transfer or share ownership of the sidecars with the RPC layer. This is not always desirable from the caller's point of view for resource management. This change fixes the problem above by modifying the RPC protocol to allow an in-flight RPC call to be aborted. Specifically, a new footer is added to all outbound call requests. It contains a flag, when true, indicates the request was cancelled mid-flight and the inbound call should be ignored altogether. This footer enables the caller to relinquish references to the sidecars early when an outbound call is cancelled or timed-out. Once the call is cancelled or timed-out, the RPC layer will send the remainder of the bytes for the request with some dummy bytes. Since the aborted flag is set in the footer, the inbound call will be ignored anyway so it's okay to send random bytes. To accommodate the new footer, a new RPC feature flag HAS_FOOTER is also introduced in this change. Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc --- M docs/design-docs/rpc.md M src/kudu/rpc/blocking_ops.cc M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/constants.cc M src/kudu/rpc/inbound_call.cc M src/kudu/rpc/inbound_call.h M src/kudu/rpc/outbound_call.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/serialization.cc M src/kudu/rpc/serialization.h M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 15 files changed, 329 insertions(+), 99 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/7598/2 -- To view, visit http://gerrit.cloudera.org:8080/7598 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho
