Adar Dembo has posted comments on this change.

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

Patch Set 6:


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.
Commit Message:

Line 10: to specify a new mehtod option when defining rpc service methods.
Nit: method
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?
File src/kudu/rpc/

Line 46: #include "kudu/common/common.pb.h"
Nit: should come before gutil.

Line 172: 
Nit: extra line?
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()?
File src/kudu/rpc/

Line 96: if (!call->header().has_request_id()) {
       :     ctx = new RpcContext(call, req.get(), resp, nullptr);
       :   } else {
       :     ctx = new RpcContext(call, req.get(), resp, 
       :   }
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 = 
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
To unsubscribe, visit

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

Reply via email to