Adar Dembo 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_);
> 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 = 
FindOrDie(client_state->completion_records,
        :                                                   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 
isolation.

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 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