Todd Lipcon has posted comments on this change.

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

Patch Set 17:

File src/kudu/rpc/

Line 54:   }
what do you think about adding a utility function to map-util like

Then the above 10 lines or so could be:

  ClientState* client_state = ComputeIfAbsent(&clients_, request_id.client_id(),
       []{ return unique_ptr<ClientState>(new ClientState()); }).get();

Line 76:     CHECK(client_state->completion_records.emplace(request_id.seq_no(),
I think you can just emplace(request_id.seq_no(), completion_record) here 
without constructing a unique_ptr and std::moving it, since unique_ptr<T> has a 
unique_ptr(T* val) constructor

(same is true above)

PS17, Line 85: means a tablet is being bootstrapped for the second time,
since this code is in the RPC system I think it would be better to be more 
general in the description here. Plus I think you decided that this isn't the 
only case in which this happens, right?

Line 123:   CompletionRecord* completion_record = cr_it->second.get();
another potential for a map-util function here

Line 153:   // ... if we did find a CompletionRecord return true if we're the 
driver of false

Line 158: void ResultTracker::LogAndTraceAndRespondSuccess(RpcContext* context,
these methods are duplicating a lot of RpcContext. Remind me again why we can't 
just call context->RespondSuccess()? (perhaps after breaking it out into one 
that takes a PB instead of using 'response_pb_'

PS17, Line 191: e(
nit: add 'Unlocked' to the name

PS17, Line 194: PREDICT_TRUE
no need for PREDICT inside CHECK (it's already implicitly part of CHECK)

Line 206:     const Message* response) {
should this be a reference? it doesn't look like it hangs onto the pointer

Line 219:   CHECK(completion_record->driver_attempt_no == 

Line 235:           completion_record->ongoing_rpcs.erase(orpc_iter.base()));
hrm, not really familiar with what this is doing. it seems odd though that you 
increment it and _then_ erase it...
File src/kudu/rpc/result_tracker.h:

Line 19: #include <google/protobuf/message.h>
can you get away with a forward decl? am guessing to do so you'll have to 
define ctor and ctor in the .cc file instead of implicit ones, but that's 
probably a good idea anyway

Line 28: #include "kudu/gutil/stl_util.h"
nit: sort

Line 35: // A ResultTracker for RPC results.
somewhere in here I think it would be worth a sentence or two saying that in 
most cases this is internal to the RPC system (ie handlers don't call the 
Respond* methods directly)

PS17, Line 38: sequence number
sequence number and client ID

PS17, Line 41: rpc
nit: s/rpc/RPC/ here and elsewhere

PS17, Line 44: id 
nit: capitalize ID here and elsewhere

PS17, Line 61: being completed,
being handled

PS17, Line 70: od 

PS17, Line 81: be the only handler,
this isn't necessarily true -- the client could have gotten a timeout and 
decided to retry while the original attempt was still running, right?

PS17, Line 84: hanlder 

Line 85: // previous leader originating update.
I think this description is missing one key thing that distinguishes this case 
from the "two retries on the same server" case. Here, we actually know a priori 
that the original leader-originated operation is the one that is going to 
succeed (it has to, because it has been committed), and therefore it needs to 
"steal" the handler role, even if it arrives after a client-originated request. 
However, in the case that you have a non-replicated operation (like two 
client-originated requests) you are still free to arbitrarily assign which one 
gets the ownership and let the other one tack on.

So this whole section is really about "stealing ownership" rather than 
"multiple handlers", right?

PS17, Line 89:  must be handled 
not sure what this means.. do you mean that the responses must be mutated 
exclusively by one handler?

PS17, Line 94: there o
missing a word

Line 182:   void RecordCompletionAndRespond(const RequestIdPB& request_id,
This and the three methods below are only called via RpcContext, right? I think 
it's worth noting for each method whether it's "user-facing" or "rpc-system 

Line 221:     int64_t driver_attempt_no;
what about if it's completed? this is the attempt number that successfully 
handled it?

Line 227:   struct ClientState {

PS17, Line 242: it's
nit: its

also, I dont know what "its own" means, exactly

Line 243:   // 2 - It's the driver of the RPC and the RPC has no handler (was 
what is "was attached"?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 17
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