Alexey Serbin 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.


Line 94:     RetriableRpcStatus status;
nit: is it possible to make status local variable for related scopes only?  It 
does not look like the status variable needs to go though all the scopes to 
accumulate some changes on its initial value.


Line 243:       SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Do we want these pseudo-random numbers to repeat in the same sequence for every 
run of the test?  If not, consider initializing random with seed, i.e. calling 
srandom(time(nullptr)) or something like that before the while() cycle.


Line 257:           rand() % (2 * FLAGS_remember_responses_ttl_msecs)));
Ditto for the rand().


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


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


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.'  ?


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


-- 
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-HasComments: Yes

Reply via email to