[kudu-CR] Add time/watermark based garbage collection to ResultTracker

2016-07-19 Thread Alexey Serbin (Code Review) 
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Add time/watermark based garbage collection to ResultTracker

2016-07-19 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

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


Patch Set 13:

Build Started http://104.196.14.100/job/kudu-gerrit/2537/

-- 
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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add time/watermark based garbage collection to ResultTracker

2016-07-19 Thread David Ribeiro Alves (Code Review) 
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3628

to look at the new patch set (#13).

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

Add time/watermark based garbage collection to ResultTracker

This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.

After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.

After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.

Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.

This adds a simple test that makes sure this basically
works, and adds a multithreaded threaded test that runs GC at the
same time as writes.

Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
R src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
10 files changed, 374 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/13
-- 
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins