David Ribeiro Alves has posted comments on this change.

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


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3190/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 40:   lock_guard<simple_spinlock> l(&lock_);
> I suspect it will be, as nearly every RPC will call CheckDuplicate(), no?
yeah, definitely something we can optimize for later, if this comes up in the 
hotspots list. we usually follow the policy of implementing a simple solution 
first and optimizing it later if needed.
Added a TODO in this regard though.


Line 78:       LOG(FATAL) << "Wrong state";
> OK, could we at least log what the wrong state was?
true, we can. Done.


Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
        :   CompletionRecord* completion_record = 
FindOrDie(client_state->completion_records,
        :                                                   sequence_number);
> Well, this goes back to the lack of documentation thing. :)
Well this is not a "new" module per se, IMO this is an change in the semantics 
of the rpc module. 
A couple of things:
1 - This is not meant to be used by itself in the majority of cases (single 
server)as this is hidden behind RpcContext. In the few cases where it's meant 
to be used stand alone (Write() for one) yes, care would have to be taken but 
if this fails what would it even reply to the client "Request lost?". Which 
takes us to point 2.
2 - This doesn't always reply to the client. For instance in for writes If a 
follower is calling this on some op that came from the leader there might not 
(and likely won't) be a client to reply to, we're just storing the response in 
the case it will eventually appears. Meaning this would fail silently if we 
didn't crash, meaning we'd have a bug we don't know exists as a new client rpc 
would be treated as new (an instance of the data corruption we have now).

I've improved the docs to reflect that this requires that CheckDuplicate() was 
called before, please take a look, but I don't want to go into too much detail 
on every possible usage of this.


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <david.al...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <david.al...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to