Adar Dembo has posted comments on this change.

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


Patch Set 3:

(25 comments)

I did a light first pass.

Overall I think this needs more documentation; I'd have absolutely no idea how 
to use a ResultTracker without 1) looking at the implementation, and 2) looking 
at the other patches. Ideally the header should give enough information for 
someone to use the class safely.

http://gerrit.cloudera.org:8080/#/c/3190/3//COMMIT_MSG
Commit Message:

Line 14: will addressing the missing functionality.
Nit: will address


Line 21: and it's not clear waht it would do) which became problematic as the
Nit: what


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_);
AFAICT lock_ only protects clients_, so don't we only need to hold it between 
L41 and L45?

Same comment applies to other usages of lock_, more or less.


Line 46:   client_state->last_heard_from = MonoTime::Now(MonoTime::COARSE);
This suggests that ClientState's constructor should not initialize 
last_heard_from, since we're guaranteed to set it here any time a new one is 
created.

Also, use FINE.


Line 55:       completion_record->ongoing_rpcs.push_back({response, context});
DCHECK that context isn't null?


Line 78:       LOG(FATAL) << "Wrong state";
Seems pretty easy to end up here if I call CheckDuplicate() again after getting 
an RpcState::NEW response. Shouldn't we handle that more gracefully than 
inducing a crash?


Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
        :   CompletionRecord* completion_record = 
FindOrDie(client_state->completion_records,
        :                                                   sequence_number);
Seems heavy-handed to die instead of returning a bad status if client_id or 
sequence_number can't be found.


Line 139:   {
Nit: don't need an extra scope here.


Line 163: 
Nit: got an extra empty line here. Check below too.


Line 188: void ResultTracker::ProcessAck(
So why include it in the patch at all?


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

Line 34: typedef rpc::RequestTracker::SequenceNumber SequenceNumber;
Hmm, doing this here means it'll leak into any includer of this header, and may 
collide with other definitions of SequenceNumber. Can we do without it? AFAICT 
you'll have to use RequestTracker::SequenceNumber below.


Line 36: // A ResultTracker for RPC results.
No real class-level comments? What about some design considerations? Or usage 
information?


Line 38: RpcContext is a template argument to allow this class to be tested 
without having to setup the
       : // whole RPC infrastructure.
Not the case anymore, right?


Line 45:   enum RpcState {
Nit: would be nice to explain in comments what the different states mean.


Line 51:   // Checks and returns the status of the RPC.
Nit: empty line before.


Line 53:   // If the RpcState is anything else it will taked care of internally.
Nit: "it will taked care of internally" --> should be "taken care of", but 
beyond that it's not really clear what it means to "take care of" the RPC. 
Could you clarify?


Line 54:   // If 'response' and 'context' are non null, they are stored so that 
we can later reply
How can they be null? Isn't that invalid?


Line 68:   // Responds to all RPCs identified 'client_id' and 'sequence_number' 
with the same response,
Nit: identified by


Line 73:   void FailAndRespond(const std::string& client_id,
Nit: overloads are typically frowned on, could you find a naming scheme for 
these three variants?


Line 86:   // Trims the state for a given client.
This is vague, can you add more detail?


Line 104:     ClientState() : last_heard_from(MonoTime::Now(MonoTime::COARSE)) 
{}
Nit: we generally default to FINE, saving COARSE for only those rare situations 
where it's needed.


Line 106:     std::map<SequenceNumber, CompletionRecord*> completion_records;
Does this map own the completion records? If so, should use unique_ptr. If not, 
should explain ownership semantics somewhere.


Line 109:   ResultTracker::CompletionRecord* GetAndDeleteCompletionRecord(
Shouldn't this return a unique_ptr, so it's clear ownership of the 
CompletionRecord is being passed to the caller?


Line 114:   void LogAndTraceFailure(RpcContext* context, const 
google::protobuf::Message& msg);
Nit: can you rename the variants to avoid overloading?


Line 119:   std::map<std::string, ClientState*> clients_;
unique_ptr, I think clients_ owns ClientStates, no?


-- 
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to