Todd Lipcon has posted comments on this change.

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

Patch Set 13:

File src/kudu/rpc/

Line 64: } // namespace
> nit: anonymous namespace, otherwise it looks like namespace name is missing

Line 94:     RetriableRpcStatus status;
> nit: is it possible to make status local variable for related scopes only? 
changed to use brace-initialization for the Status structs at each return site

Line 120:     status.result = RetriableRpcStatus::OK;
hrm, I'm curious why David removed this 'still report errors' bit here. will 
try to dig

Line 243:       SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
> Do we want these pseudo-random numbers to repeat in the same sequence for e
added SeedRandom() to test setup

Line 257:           rand() % (2 * FLAGS_remember_responses_ttl_msecs)));
> Ditto for the rand().
since it's multi-threaded, I don't know that a specific random seed will 
actually make it use the same sequence, but the above SeedRandom() I added 
should help for any single-threaded test cases

Line 270:   // Stubbornly sends the same request to the server, this should 
observe three states
> Nit: period is missing in the end of the sentence.

Line 271:   // The request should be successful at first, then its result 
should be gc'd and the the
> typo: strayed extra 'the'.

Line 297:         SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
> Ditto for the rand().

Line 513: // This test creates a thread continuously makes requests to the 
server, some lasting longer
> nit: makes --> making
File src/kudu/rpc/result_tracker.h:

Line 262:     // The timestamp of the last time this CompletionRecord was 
> nit: 'The timestamp of the last CompletionRecord update.'  ?

Line 301:     SequenceNumber stale_before_seq_no;
> Is it OK to leave it uninitialized in the constructor (I didn't see it was 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Alexey Serbin <>
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