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 <k...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
