David Ribeiro Alves has posted comments on this change.

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

Patch Set 13:


File src/kudu/rpc/result_tracker.cc:

Line 53:     CHECK(clients_.emplace(request_id.client_id(), 
> PREDICT_TRUE would be nice documentation value here that this is the hot pa

Line 71:       completion_record->ongoing_rpcs.push_back({response, 
> this block might be somewhat expensive to do inside the lock... I guess it'
true on both counts. I'd like to, in time and when we have more tests for this, 
have per state locking.

Line 72:     }
> reaching into the guts of RpcContext seems a little odd here.
yeah, the reason for that is that context will wrap the result tracker, so if 
we call the external api methods of the context here we'd have a cycle.

PS12, Line 146: 
> nit: missing space

Line 176:     << " was not marked as the driver for the RPC.";
> dumping the response stringified here would be useful for debugging such a 

File src/kudu/rpc/result_tracker.h:

Line 41: // If the client wishes to track the result of a given rpc it must 
send on the rpc header
> is this class entirely internal to the RPC system or would it be used by se
this is internal to the RPC system for non-replicated rpc's, but needs to be 
used ad-hoc for replicated ones (writes, etc...)

PS12, Line 90: 
> I think this one-line summary is a bit inaccurate, since it actually will _
I can change this to something like TrackRpc(), would that make sense? I don't 
want to call it "Start" since it might not be the start of that rpc. I'll add 
some info to the comment below to make it clear that for anything but NEW, the 
passed context and response will be taken over

Line 98:     // collection watermark and we do not recall the original response.
> What if you broke out the guts of this method, so this just returned the Rp
then the door is open to TOCTOU situations. See a follow up patch for the 
interaction of this with the transaction driver.

PS12, Line 107: 
> nit: of

Line 108:   // The caller still owns the passed 'response' and 'context'.
> clarify: on the same server, right?

PS12, Line 112: ontext'.
> why does RPC2 call FailAndRespond here? not following
RPC2 (which here means an RPC from another replica) calls FailAndRespond() to 
"cancel" the client's RPC and establish itself as the current driver of this 
request. See comment on transaction_driver.h on the follow up patch that 
integrates this with writes (this is only used for replicated rpcs)

Line 114:                     google::protobuf::Message* response,
> still confused: I thought we just called FailAndRespond, why do we now call
RPC2 calls FailAndRespond() to cancel RPC1 and establish itself as the driver.

Line 124:   // from another replica and should take precedence:
> I remember discussing a scenario like this before I went on vacation, but I
I've added some more info that hopefully makes this clearer. This is this is 
super specific to replicated ops and has no relevance for single server ops. 
I've added a very detailed comment to transaction_driver.h on a follow up patch 
that hopefully clears up why and when this is needed.

PS12, Line 128: d(), cance
> typo

PS12, Line 128: ef
> nit: no comma

PS12, Line 129:  is still alive.
> retries of the same RPC?

Line 133:   // 6 - RPC2 - Also completes. The RPC has now been executed twice.
> I wonder whether this could be "hooked" in the RpcContext in such a way tha
that's exactly what https://gerrit.cloudera.org/#/c/3192/11 does. for 
non-replicated rpcs server code never touches the result tracker.

PS12, Line 167: 
> this 'driver' term shows up in this class a bit but seems new. is it in the
this is kind of new and was needed to address that racy situation that we 
talked about a while ago. Added some more info in the header to make this clear.

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

Line 176:   struct OnGoingRpcInfo {
> can you make it a map of unique_ptr<> now that we have c++11? then you don'

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

PS12, Line 185: received RpcState::
> naming is inconsistent with the pattern above

PS12, Line 193: from;
> unique_ptr<ClientState>?

Line 194:     std::map<SequenceNumber, std::unique_ptr<CompletionRecord>> 

To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to