David Ribeiro Alves has posted comments on this change.

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

Patch Set 3:


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

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 betwe
no, the lock protects access to clients_ and to the ClientState that belongs to 
each client. If we deem this a hotspot we can improve by keeping a per 
ClientState lock, but I'd rather do that later, if needed.

Line 46:   client_state->last_heard_from = MonoTime::Now(MonoTime::COARSE);
> This suggests that ClientState's constructor should not initialize last_hea
right, Done

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 get
the whole point is that we only call this once per RPC, if the caller got NEW 
before it should have just executed the RPC, there is no reason to call this 

Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
        :   CompletionRecord* completion_record = 
        :                                                   sequence_number);
> Seems heavy-handed to die instead of returning a bad status if client_id or
why? if we are responding to an RPC that is not being tracked then it's an 
error. I'd rather catch those through a crash since it's incorrect server (not 
client) behavior

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?
true, removed

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
moved it inside the ResultTracker.

Line 36: // A ResultTracker for RPC results.
> No real class-level comments? What about some design considerations? Or usa
true, my bad. In my defense this was kind of wip-py and wanted to get the first 
round of comments. Note that this is not a class for general use. For the big 
majority of RPCs this sits behind an RpcContext and server code never has to 
use it directly.
This being said I did expand this comment to reflect normal usage and lifecycle.

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 

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?
expanded the class comment to cover this.

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 just correspond to the signatures in RpcContext, which also overloads, so 
I think we should keep them the same.
the exception is RespondApplicationError() which I guess we could change, but I 
don't think that includes any more information.

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 situat
Done, though AFAICT not sure that's totally true. For instance scanner TLL is 
measured with COARSE and it's not performance sensitive

Line 106:     std::map<SequenceNumber, CompletionRecord*> completion_records;
> Does this map own the completion records? If so, should use unique_ptr. If 
it does, but honestly I think that the ownership semantics are pretty clear and 
this will sit in the critical path, so I'd prefer to avoid the extra pointer 
indirection. I don't feel super strong about it though, I just don't think it 
buys us much here.

Line 109:   ResultTracker::CompletionRecord* GetAndDeleteCompletionRecord(
> Shouldn't this return a unique_ptr, so it's clear ownership of the Completi

Line 114:   void LogAndTraceFailure(RpcContext* context, const 
google::protobuf::Message& msg);
> Nit: can you rename the variants to avoid overloading?
don't know how to name them honestly, just have overloads corresponding to 
whatever is in RpcContext (whose naming criteria I don't understand fully 
either). Suggestions?

Line 119:   std::map<std::string, ClientState*> clients_;
> unique_ptr, I think clients_ owns ClientStates, no?
again, would rather avoid the extra pointer indirection, if that's ok.

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