Todd Lipcon has posted comments on this change.

Change subject: Integrate the result tracker with writes

Patch Set 17:

File src/kudu/client/

Line 51: #include "kudu/rpc/rpc_header.pb.h"
File src/kudu/consensus/consensus.proto:

Line 182:   optional rpc.RequestIdPB request_id = 100;
why using such a high id here? seems like it wastes a byte
File src/kudu/rpc/

Line 162:     completion_record->final_driver = true;
this is a big code smell -- this function should be read-only, not somehow have 
a side effect. If you want it to have a side effect, rename it to 
'BecomeFinalDriver' or something? 'UpgradeToFinalDriver'?

Also, the docs in the header file should reference this concept of being the 
'final driver'
File src/kudu/rpc/result_tracker.h:

Line 226:     bool final_driver = false;
doc this

also, weird that this field has a default but the others don't
File src/kudu/tablet/

Line 251:                              WriteResponsePB* response);
mentioned on rev 1, but I dont like the side effect here without an 
accompanying method rename and doc update

Line 1243:   if (tracking_results) {
any way to extract this new bit of code to a separate function?

Line 1279:       LOG(FATAL) << "Couldn't change bootstrapping txn to driver 
after 10 attempts for request: "
this seems like a kind of arbitrary thing... should this be a DFATAL and keep 
File src/kudu/tablet/

Line 130:                         const scoped_refptr<ResultTracker> 
File src/kudu/tablet/transactions/

Line 212:   if (state()->are_results_tracked()) {
change to if (!...) return

Line 314:     case REPLICATING:
some spurious changes here
File src/kudu/tablet/transactions/transaction_driver.h:

PS17, Line 117: // 1 - When becoming leader, a replica has already prepared all 
the requests that it received
              : //     from the previous leader that it will try to 
replicate/commit itself.
              : /
not quite understanding this sentence

PS17, Line 122: Requests originated on other replicas 
not sure quite what you mean here. Do you mean operations which were started 
while the node is a follower? Or "originated" meaning "the first attempt"?

PS17, Line 158: FailAndRespond() 
hrm, is this FailAndRespond here actually responding to any RPCs in the current 
design? Can we encapsulate this "precedence" behavior into something like the 
'UpgradeToExclusiveDriver' call I mentioned in the other review?
File src/kudu/tablet/transactions/write_transaction.h:

PS17, Line 196: c_
File src/kudu/tserver/

Line 137:                                  dummy,
can just use scoped_refptr<rpc::ResultTracker>(), no?
File src/kudu/tserver/

Line 280: const char* ALREADY_PRESENT_ERROR = "There's already an attempt at 
the same operation "
is this used?

Line 295:       //LOG(INFO) << "Responding error to: " << 

Line 764:   // attempted elsewhere.
this probably needs to be moved into tablet_peer

To view, visit
To unsubscribe, visit

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

Reply via email to