Todd Lipcon has posted comments on this change.

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


Patch Set 13:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:

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


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.
Done


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


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


Line 513: // This test creates a thread continuously makes requests to the 
server, some lasting longer
> nit: makes --> making
Done


http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

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


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


-- 
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to