Todd Lipcon has posted comments on this change.

Change subject: Integrate the ResultTracker into the rpc subsystem and add a 

Patch Set 11:

File src/kudu/common/common.proto:

Line 316:   optional bool track_rpc_result = 50006 [default=false];
I dont think this should be in 'common'. 'common' is for data-model type stuff. 
Perhaps 'rpc_header.proto' or a new proto file in the rpc/ subdir. (rpc doesn't 
depend on common, so actually slightly surprised this builds properly)
File src/kudu/rpc/CMakeLists.txt:

Line 89:     kudu_common_proto
oh, now I see why it builds. This dependency seems misplaced. (eg keep in mind 
that Impala's thinking about using krpc, so we shouldn't have upwards 
dependencies from RPC into common/)
File src/kudu/rpc/

Line 46: #include "kudu/common/common.pb.h"
nit: sorting

Line 467:               "    mi->result_tracker.reset($result_tracker$);\n"
we have a separate ResultTracker per RPC type? that doesn't seem right.
File src/kudu/rpc/

PS11, Line 73: random amount 

Line 134:     vector<ExactlyOnceAdder*> adders;
use unique_ptr?
File src/kudu/rpc/rpc-test-base.h:

Line 288:   std::atomic_int exactly_once_test_val_;
hrm, is this atomic<int>? haven't seen this
File src/kudu/rpc/

PS11, Line 66:     call_->RespondSuccess(*response_pb_);
             :     delete this;
per the comment on the other review, it seems weird we now have this code split 
between ResultTracker and here.
File src/kudu/rpc/rtest.proto:

Line 129:     option (kudu.track_rpc_result) = true;
add something to design-docs/ about this feature?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to