David Ribeiro Alves has posted comments on this change.

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


Patch Set 12: Verified+1

(12 comments)

http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG
Commit Message:

Line 10: to specify a new method option when defining rpc service methods.
> I think you missed this.
hrm, you're right. I could swear I had fixed this. must have lost a patch along 
the way. Done


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

Line 316
> I dont think this should be in 'common'. 'common' is for data-model type st
Done


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

Line 89:     rpc_header_proto
> oh, now I see why it builds. This dependency seems misplaced. (eg keep in m
Done


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/rpc/rpc_header.pb.h"
> nit: sorting
Done


Line 467:               "    mi->result_tracker.reset($result_tracker$);\n"
> we have a separate ResultTracker per RPC type? that doesn't seem right.
we do, why doesn't it seem right? easier to track where memory goes , maps will 
be smaller so faster. locks will be less contended... why wouldn't we want to 
have one per rpc?


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
Done


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


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
yeah it's typedefd, want me to change it?


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 s
well yeah, for the case when the result aren't being tracked. ideally we'd like 
to at least coalesce the log statements at least, was kinda bugging me but 
didn't spend too much time on it. Any ideas?


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

Line 129:   }
> add something to design-docs/rpc.md about this feature?
Done


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

PS6, Line 96:                                  req.release(),
            :                                    resp,
            :                                    
call->header().has_request_id() ? method_info->result_tracker :
            :                                                                   
   nullptr);
            : 
> Missed this?
Done


Line 116:   } else {
> Log the state too.
Done


-- 
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: 12
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