Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 

Patch Set 2:


Commit Message:

PS1, Line 9: call times out
> worth specifying "times out after transmission of the request has begun but

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 

PS1, Line 20: fixes the
> not in-flight but mid-transmission. We still don't support a preemptive "ab

PS1, Line 22: . It c
> nit: again I think "mid-transmission" is clearer since we usually call an R

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

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.

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

Line 448:     DCHECK_EQ(call_from_map, call_.get());
> warning: parameter 'status' is unused [misc-unused-parameters]

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 

PS1, Line 649:   const set<RpcFeatureFlag>& required_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' 
             :     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_, 
             :     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.

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

Line 199:   // before negotiation completes. Callers should takes this into 
> use ContainsKey from map-util.h
Done. Also added negotiation_complete_ &&

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)

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, 
             :   }
> 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().

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 

Line 168: 
> warning: function 'kudu::rpc::OutboundCall::RelocateSidecars' has a definit

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

File src/kudu/rpc/rpc-test.cc:

Line 994:     
controller.set_timeout(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 
> warning: either cast from 'int' to 'uint64_t' (aka 'unsigned long') is inef

Line 994:     
controller.set_timeout(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 
> maybe 30L here would help

Line 999:     SleepFor(MonoDelta::FromMicroseconds(rand.Uniform64(i * 500L + 
> warning: either cast from 'int' to 'uint64_t' (aka 'unsigned long') is inef

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

PS1, Line 98: 
> let's be more specific with this name like 'REQUEST_FOOTERS' in case we dec

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

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.

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

File src/kudu/rpc/transfer.cc:

Line 56: using std::ostringstream;
> warning: using decl 'set' is unused [misc-unused-using-decls]

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

Reply via email to