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<RpcFeatureFlag>& 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 need to repeat this here since it's in the PB definition (or should be) Done http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: Line 90: } > this doesn't seem right... we should be negotiating this early during the c Fixed. The new behavior is that the client always advertises REQUEST_FOOTERS in its feature flags. Servers which support parsing footer will set REQUEST_FOOTERS if a client has that feature too. PS1, Line 143: *slice_iter++ = Slice(relocated_dst, sidecar->AsSlice().size()); : } > can you grab the length before and after this and CHECK that they are the s Very good point. Done. Line 438: } > I'm surprised to see ON_OUTBOUND_QUEUE here actually. maybe a better name w Renamed to IsBeingSent(). http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS1, Line 166: // in the first 4 bytes of "header_buf_") is incremented to account for the footer. : void AppendFooter(Slice* slice); > I find it surprising that this method has this side effect, based on the na I renamed this MarkPayloadCancelled(). Hope it's more clear about its purpose now. Line 168: > warning: function 'kudu::rpc::OutboundCall::RelocateSidecars' has a definit Done PS1, Line 206: IsCancelle > how about 'IsBeingSent' or 'TransmissionInProgress' or somesuch? Renamed to IsBeingSent(). PS1, Line 316: : // The remote method being called. : RemoteMethod remote_method_; > don't think this is necessary here Done http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: Line 994: controller.set_timeout(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 1))); > warning: either cast from 'int' to 'uint64_t' (aka 'unsigned long') is inef Done Line 994: controller.set_timeout(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 1))); > maybe 30L here would help Done Line 999: SleepFor(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 1))); > warning: either cast from 'int' to 'uint64_t' (aka 'unsigned long') is inef Done http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: PS1, Line 96: // See RequestFooterPB below. : REQUEST_FOOTERS = 4; > I think these two lines are redundant with just looking at the RequestFoote Done PS1, Line 98: > let's be more specific with this name like 'REQUEST_FOOTERS' in case we dec Done Line 273: // This flag indicates whether the incoming request has been aborted by the client > I know we aren't very consistent here in this file, but can you rename this Done PS1, Line 274: // and t > - why make this required? what if some day we want to deprecate it? Switched to optional. It's true that we could deprecate this flag if there is a big design change in the future although the footer may become empty when all fields are optional. Added some comments too. http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/serialization.cc File src/kudu/rpc/serialization.cc: PS1, Line 113: DCHECK_E > seems safe enough to be a DCHECK here since this is really just an invarian Done http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: Line 56: using std::ostringstream; > warning: using decl 'set' is unused [misc-unused-using-decls] Done PS1, Line 68: g_dummy_buff > style: rename to g_dummy_buffer since it's a global. Great idea ! Thanks for the pointer to OverwriteWithPattern(). That saved me from re-inventing the wheel :-). Line 194: if (!TransferFinished() && !aborted_) { > style: we usually prefer "early return" vs nesting the whole function. ie i Done. I noticed that there are two bugs in PS1 while modifying it: - if the footer's slice has been sent partially already (however rare that the case may be), we should just treat it as "finished" and let the remaining bytes be sent. The transfer callback should still have a shared_ptr to the outbound call so it should still be safe to keep accessing footer_buf_ and the slices in an OutboundCall object. - RelocateSidecars() adjust the number of slices without updating cur_slice_idx_. The new patch fixes it by not adjusting the number of slices. Also made n_payload_slices_ constant. Line 213: return; > this loop is certainly tricky... do you have confidence that it's all cover Yes, I used a 16MB buffer for sidecar and randomize the size of the sidecar. PS2 also extends the test to randomize the number of sidecars. -- 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
