David Ribeiro Alves has posted comments on this change.

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

Patch Set 17:


File src/kudu/rpc/result_tracker.cc:

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

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 

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/

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

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

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 

Line 219:   CHECK(completion_record->driver_attempt_no == 

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.

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

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

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

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

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

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

PS17, Line 61: being completed,
> being handled

PS17, Line 70: od 
> methods

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

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

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

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

Line 243:   // 2 - It's the driver of the RPC and the RPC has no handler (was 
> 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