Todd Lipcon has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results

Patch Set 12:

File src/kudu/rpc/

Line 53:   if (completion_record == nullptr) {
PREDICT_TRUE would be nice documentation value here that this is the hot path

Line 71:       DCHECK_NOTNULL(response)->CopyFrom(*completion_record->response);
this block might be somewhat expensive to do inside the lock... I guess it's OK 
because we assume this is a rare code path?

Line 72:       DCHECK_NOTNULL(context)->call_->RespondSuccess(*response);
reaching into the guts of RpcContext seems a little odd here.

PS12, Line 146: "
nit: missing space

Line 176:   CHECK(completion_record->response == NULL) << "This request was 
previously marked as successful.";
dumping the response stringified here would be useful for debugging such a crash

Line 221: }
lots of dup code in the above methods. any refactor doable? (even one that 
takes a lambda of what to do with each ongoing_rpc)?
File src/kudu/rpc/result_tracker.h:

Line 41: // When a call first arrives from the client the RPC subsystem will 
call CheckDuplicate() which
is this class entirely internal to the RPC system or would it be used by 
service implementors as well? I think some of my questions below might be moot 
based on your answer to this question.

PS12, Line 90: returns the status
I think this one-line summary is a bit inaccurate, since it actually will 
_handle_ the RPC in the case that it's a duplicate. Also, the name is 
inaccurate in the same way (see below for a suggestion)

Also, this registers the RPC as currently running, right? maybe 'StartRPC' is a 
better name? Should document whether there is any 'cleanup' that the caller 
needs to do here to avoid leaking it as an in-progress RPC.

Line 98:   RpcState CheckDuplicate(const RequestIdPB& request_id,
What if you broke out the guts of this method, so this just returned the 
RpcState, and you had a new method 'HandleIfDuplicate' which does what this 
method is doing?

Then you should be able to unit test this without mocking RpcContext?

PS12, Line 107: at 
nit: of

Line 108:   // RPC:
clarify: on the same server, right?

PS12, Line 112: FailAndRespond()
why does RPC2 call FailAndRespond here? not following

Line 114:   // 4 - RPC2 - Calls CheckDuplicate(), gets RpcState::NEW back.
still confused: I thought we just called FailAndRespond, why do we now call 
CheckDuplicate again?

Line 124:   // sure their attempt number is still the driver of the RPC and 
give up if it isn't.
I remember discussing a scenario like this before I went on vacation, but I 
can't quite remember the details, and the above explanation isn't quite clear 
to me. Perhaps a more concrete example (and some more clarity on the threads 
involved) would be useful?

PS12, Line 128: with from 

PS12, Line 128: , 
nit: no comma

PS12, Line 129: attempts at this RPC 
retries of the same RPC?

Line 133:   void RecordCompletionAndRespond(const RequestIdPB& request_id,
I wonder whether this could be "hooked" in the RpcContext in such a way that it 
would happen automatically? ie once you associate an RpcContext with a 
retriable RPC, _any_ response to the RPC would need to record completion to 
avoid a leak and potential dangling reference, no? I'm worried that relying on 
the handlers to use this special 'record completion' might be tricky? (or is 
this called from RpcContext itself?)

PS12, Line 167: unique identifier 
where does this identifier come from? not sure what it corresponds to.

PS12, Line 167: driver
this 'driver' term shows up in this class a bit but seems new. is it in the 
design doc somewhere and I missed it?

Line 176:     ~ClientState() { STLDeleteValues(&completion_records); }
can you make it a map of unique_ptr<> now that we have c++11? then you don't 
need a ctor (and this struct would be safely moveable, whereas right now there 
would be a bug if you copied ClientState)

PS12, Line 181: DeleteC
Remove is probably better than Delete since it actually gets returned

PS12, Line 185: LogAndTraceFailure(
naming is inconsistent with the pattern above

PS12, Line 193: ClientState*>

Line 194: };

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to