Todd Lipcon has posted comments on this change.

Change subject: Integrate the result tracker with writes

Patch Set 17:


Here a bunch of comments I had pending on PS1 (starting to review again now the 
latest patch set, but figured I'd publish these, even if they are irrelevant 
Commit Message:

Line 12: - Support for caching responses for follower transactions.
is there any support for eviction yet? if not, does this patch make it 
off-by-default? otherwise this is just a memory leak?
File src/kudu/client/

Line 304:   replica->proxy()->WriteAsync(req_, &resp_,
worth a note why this is duplicated in the WriteRequestPB
File src/kudu/integration-tests/

Line 243:         CHECK(!overflow);
since this is a thread, we can't use FAIL(). Maybe we can just use 
FlushSessionOrDie() in this code path instead?
File src/kudu/rpc/

PS1, Line 71: y exist.
whats "for the second time"?
File src/kudu/rpc/

PS1, Line 108:    met
also seems kind of funky to have a random hex ID here..
File src/kudu/tablet/

Line 110:         scoped_refptr<rpc::ResultTracker>(),
can we get some follow-up JIRAs filed for some of these tests? eg a test here 
which validates that the bootstrap populates the cache
File src/kudu/tablet/

Line 1177:   // Set up the ConsensusBootstrapInfo structure for the caller.
hacking this into this method seems like the wrong place (it's more of a 
read-only method, but now it has a weird side effect)

even if we have to loop over the ops a second time I think clarity of code is 
better than trying to consolidate the loops

Line 1214: 
this seems like it will slow down bootstrap noticeably... can we short-circuit 
this based on the time retention? (feel free to add a TODO)

Line 1234:   tablet_->StartApplying(&tx_state);
weird, shouldn't this be after it actually applies?
also, we seem to have lost the timestamp in the WriteResponsePB. I'm worried 
we'll lose other fields in the future if we aren't really careful about this.

eg as a thought exercise: what if we had a 'getAndIncrement' write type with 
associated results in the WriteResponsePB. How would those end up tracked?
File src/kudu/tablet/transactions/transaction_driver.h:

Line 133: //             a) ---------                                  b) 
I think this diagram would be clearer if the two weren't horizontally next to 
each other... because this looks like it's actually a top-to-bottom sequence 
diagram with two threads 'a' and 'b' instead of two different diagrams
File src/kudu/tserver/

Line 123: const char* WRITE_METHOD_NAME = "Write";
what's this used for? dont see it referenced elsewhere

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