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-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 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
