Todd Lipcon has posted comments on this change. Change subject: Add time/watermark based garbage collection to ResultTracker ......................................................................
Patch Set 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/integration-tests/exactly_once_writes-itest.cc File src/kudu/integration-tests/exactly_once_writes-itest.cc: PS15, Line 84: cncurrently > concurrently + period Done PS15, Line 86: > word missing Done PS15, Line 95: For > Extra word? Something is off in this sentence. does a comma after 'batches' make it read better? http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/exactly_once_rpc-test.cc File src/kudu/rpc/exactly_once_rpc-test.cc: Line 40: unique_ptr<RequestIdPB> request_id(new RequestIdPB()); > So I know this is a little late to the game, but RpcController holding it's yea, it's a little strange. want to file a follow-up JIRA? It's baked throughout a bunch of places here. http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/result_tracker.h File src/kudu/rpc/result_tracker.h: Line 225: // Runs garbage collection on the results this result tracker is caching. > I would change this to 'Runs time-based garbage collection...' to distingui Done PS15, Line 307: MustGcRecordFunc func > Ideally this would be a template param instead of a std::function so that i Done http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: Line 126: required int64 first_incomplete_seq_no = 3; > All of these fields should be optional, we shouldn't be introducing any mor why? given this struct itself is optional, isn't it safe for its first introduction to have required fields? In this case though I guess I agree that semantically it's fine for it to be missing (it just means no GC). I'm guessing David made it required so that we dont have cases with misbehaving clients which don't set it and thus end up causing the server to exhaust a lot of memory? http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 1253: || state == ResultTracker::RpcState::COMPLETED > || on previous lines 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: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@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
