Todd Lipcon has posted comments on this change.

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


Patch Set 11:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/common/common.proto
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)


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/CMakeLists.txt
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/)


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:

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.


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:

PS11, Line 73: random amount 
update


Line 134:     vector<ExactlyOnceAdder*> adders;
use unique_ptr?


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc-test-base.h
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


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rpc_context.cc
File src/kudu/rpc/rpc_context.cc:

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.


http://gerrit.cloudera.org:8080/#/c/3192/11/src/kudu/rpc/rtest.proto
File src/kudu/rpc/rtest.proto:

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


-- 
To view, visit http://gerrit.cloudera.org:8080/3192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 11
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