David Ribeiro Alves has posted comments on this change.

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


Patch Set 17:

(29 comments)

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

Line 54:   }
> what do you think about adding a utility function to map-util like https://
Done


Line 76:     CHECK(client_state->completion_records.emplace(request_id.seq_no(),
> I think you can just emplace(request_id.seq_no(), completion_record) here w
created a ComputeIfAbsenseAndReturnAbsense function that works in the lines of 
the one you suggested but also returns wether a new value was created.


PS17, Line 85: means a tablet is being bootstrapped for the second time,
> since this code is in the RPC system I think it would be better to be more 
Done


Line 123:   CompletionRecord* completion_record = cr_it->second.get();
> another potential for a map-util function here
added FindPointeeOrNull on another patch


Line 153:   // ... if we did find a CompletionRecord return true if we're the 
driver of false
> s/of/or/
Done


Line 158: void ResultTracker::LogAndTraceAndRespondSuccess(RpcContext* context,
> these methods are duplicating a lot of RpcContext. Remind me again why we c
because if the results are tracked, the RpcContext calls this, so calling that 
would create a cycle


PS17, Line 191: e(
> nit: add 'Unlocked' to the name
Done


PS17, Line 194: PREDICT_TRUE
> no need for PREDICT inside CHECK (it's already implicitly part of CHECK)
Done


Line 206:     const Message* response) {
> should this be a reference? it doesn't look like it hangs onto the pointer
we use pointers to the response all over, think we should keep it a pointer for 
consistency


Line 219:   CHECK(completion_record->driver_attempt_no == 
request_id.attempt_no());
> CHECK_EQ
Done


Line 235:           completion_record->ongoing_rpcs.erase(orpc_iter.base()));
> hrm, not really familiar with what this is doing. it seems odd though that 
we're iterating in reverse, as not to delete the response we're copying from 
before we use it where needed. will add a note in that regard.


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

Line 19: #include <google/protobuf/message.h>
> can you get away with a forward decl? am guessing to do so you'll have to d
Done


Line 28: #include "kudu/gutil/stl_util.h"
> nit: sort
Done


Line 35: // A ResultTracker for RPC results.
> somewhere in here I think it would be worth a sentence or two saying that i
Done


PS17, Line 38: sequence number
> sequence number and client ID
Done


PS17, Line 41: rpc
> nit: s/rpc/RPC/ here and elsewhere
Done


PS17, Line 44: id 
> nit: capitalize ID here and elsewhere
Done


PS17, Line 61: being completed,
> being handled
Done


PS17, Line 70: od 
> methods
Done


PS17, Line 81: be the only handler,
> this isn't necessarily true -- the client could have gotten a timeout and d
I make a distinction between RPC (a context, request and response, may or may 
not have a handler) and a handler (a thread actually executing it). I've added 
a little glossary at the beginning.


PS17, Line 84: hanlder 
> typo
Done


Line 85: // previous leader originating update.
> I think this description is missing one key thing that distinguishes this c
I make the distinction that a handler is a thread executing an attempt, and the 
"driver" is the only handler that should be allowed to complete. Hopefully I've 
made this clear in the glossary.


PS17, Line 89:  must be handled 
> not sure what this means.. do you mean that the responses must be mutated e
I mean that only the driver handler must reply to attached attempts.
reworded this.


PS17, Line 94: there o
> missing a word
Done


Line 182:   void RecordCompletionAndRespond(const RequestIdPB& request_id,
> This and the three methods below are only called via RpcContext, right? I t
not sure what you mean. for unreplicated RPCs all the mehtods are internal, for 
replicated ones (almost) all are used from the outside.


Line 221:     int64_t driver_attempt_no;
> what about if it's completed? this is the attempt number that successfully 
yeah, it is


Line 227:   struct ClientState {
> doc
Done


PS17, Line 242: it's
> nit: its
Done


Line 243:   // 2 - It's the driver of the RPC and the RPC has no handler (was 
attached).
> what is "was attached"?
added attached to the class header docs


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

Reply via email to