Adar Dembo has posted comments on this change.

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

Patch Set 3:

File src/kudu/rpc/

Line 40:   lock_guard<simple_spinlock> l(&lock_);
> no, the lock protects access to clients_ and to the ClientState that belong
I suspect it will be, as nearly every RPC will call CheckDuplicate(), no?

Line 78:       LOG(FATAL) << "Wrong state";
> the whole point is that we only call this once per RPC, if the caller got N
OK, could we at least log what the wrong state was?

Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
        :   CompletionRecord* completion_record = 
        :                                                   sequence_number);
> why? if we are responding to an RPC that is not being tracked then it's an 
Well, this goes back to the lack of documentation thing. :)

You're designing a module with "hard" boundaries. That is, if one of us misuses 
the ResultTracker, it crashes.

Without context and in isolation, I tend to assume new modules should have 
"soft" boundaries, where misuse leads to returned errors. And to be fair, there 
are advantages to soft boundaries: it's easier to test such a module in 

Anyway, there's nothing inherently wrong with hard boundaries, you just need to 
be very clear in the interface regarding what happens in the event of misuse.

To view, visit
To unsubscribe, visit

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