Todd Lipcon has posted comments on this change.

Change subject: Integrate the ResultTracker into the rpc subsystem
......................................................................


Patch Set 19:

(7 comments)

I think this commit would make more sense if you also pulled in the changes to 
RetriableRpc from the 'integrate with client', and then merged in the new test 
cases as well, so it's a standalone change to the RPC system along with its 
tests

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

PS19, Line 11: specificed
typo


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

Line 111:   kudu_common
is this still needed? this back-dependency strikes me as evil


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

Line 189:       (*map)["result_tracker"] = "result_tracker";
how about: (*map)["result_tracker"] = track_result ? "result_tracker" : 
"nullptr"

?


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

Line 23: #include "kudu/rpc/result_tracker.h"
forward decl


Line 181:   // If this returns true, both GetResultTracker() request_id() 
should return non
- missing word "and"
- hyphenate "non-null"


PS19, Line 186: Get
style: no "Get"


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

Line 58:   scoped_refptr<ResultTracker> result_tracker;
now that the result tracker is service-wide, does it really make sense to have 
here vs just having a bool and grabbing the member of the service class? 

if not, another thought: RpcContext::ResultTracker() can call through to this 
class to grab the method info (and avoid an extra ref/deref on the 
ResultTracker ref count)


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