[kudu-CR] Add time/watermark based garbage collection to ResultTracker
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
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
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