Adar Dembo has posted comments on this change. Change subject: Integrate the ResultTracker into the rpc subsystem and add a test ......................................................................
Patch Set 6: (9 comments) It's becoming increasingly clear to me that Todd needs to take a deep look at this series; there's too much here that I don't know. http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG Commit Message: Line 10: to specify a new mehtod option when defining rpc service methods. Nit: method http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/common/common.proto File src/kudu/common/common.proto: Line 313: // An option for RPC methods that allows to set whether that method's Why define this here and not in rpc_header.proto? Wouldn't doing that mean proto-gen-krpc won't depend on kudu_common? http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 46: #include "kudu/common/common.pb.h" Nit: should come before gutil. Line 172: Nit: extra line? http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: Line 178: bool AreResultsTracked() const { return result_tracker_.get() != nullptr; } I don't think this is necessary given the below method, which is very easy to use to test for the presence of a tracker. Line 180: const scoped_refptr<ResultTracker> GetResultTracker() const { Shouldn't this return a cref and return the result_tracker_ as-is, instead of calling get()? Also, call it result_tracker()? http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: Line 96: if (!call->header().has_request_id()) { : ctx = new RpcContext(call, req.get(), resp, nullptr); : } else { : ctx = new RpcContext(call, req.get(), resp, method_info->result_tracker); : } Convert into a ternary: RpcContext* ctx = new RpcContext(call, req.get(), resp, call->header().has_request_id() ? method_info->result_tracker : nullptr); Line 103: ResultTracker::RpcState state = ctx->GetResultTracker()->CheckDuplicate( It's interesting that 'req' isn't considered at all in CheckDuplicate(). We're relying entirely on seq_no() instead, I believe. Line 116: LOG(FATAL) << "Unknown state."; Log the state too. -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <david.al...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
