Adar Dembo has posted comments on this change.

Change subject: Add time/watermark based garbage collection to ResultTracker

Patch Set 17:


It's pretty tough to review since gerrit is showing it 
as all new. Any chance you can break out the rename of the test into a separate 
patch? Alternatively, you can describe what's actually changed in the test and 
I can focus on that.
File src/kudu/rpc/CMakeLists.txt:

Line 121: ADD_KUDU_TEST(exactly_once_rpc-test)
Can you resort the tests?
File src/kudu/rpc/

PS17, Line 152:         completion_record->state = RpcState::IN_PROGRESS;
              :         completion_record->driver_attempt_no = 
Can we add these to the CR constructor?

Line 232:   completion_record->last_updated = MonoTime::Now();
Technically we haven't mutated the CR, so why update its timestamp?

Line 348:   completion_record->last_updated = MonoTime::Now();
Likewise, shouldn't we push this down into where we actually update the CR? I 
think that's L365, though I'm not sure.
File src/kudu/tablet/

PS17, Line 1247:     // We only replay committed requests so the result of 
tracking this request can be:
               :     // NEW - This is a previously untracked request, or we 
changed the driver -> store the result
               :     // COMPLETED - We've bootstrapped this tablet twice, and 
previously stored the result -> do
               :     //             nothing.
Update this comment.
File src/kudu/tablet/transactions/

PS17, Line 227:       VLOG(1) << state()->result_tracker() << " Follower Rpc 
was not NEW or IN_PROGRESS: "
              :           << rpc_state << " OpId: " << 
              :           << " RequestId: " << 
Update this too? Should have been "not NEW or COMPLETED", I think.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to