Todd Lipcon has posted comments on this change.

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

Patch Set 15:

File src/kudu/integration-tests/

PS15, Line 84: cncurrently
> concurrently + period

PS15, Line 86:  
> word missing

PS15, Line 95: For
> Extra word?  Something is off in this sentence.
does a comma after 'batches' make it read better?
File src/kudu/rpc/

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.
File src/kudu/rpc/result_tracker.h:

Line 225:   // Runs garbage collection on the results this result tracker is 
> I would change this to 'Runs time-based garbage collection...' to distingui

PS15, Line 307: MustGcRecordFunc func
> Ideally this would be a template param instead of a std::function so that i
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?
File src/kudu/tablet/

Line 1253:               || state == ResultTracker::RpcState::COMPLETED
> || on previous lines

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to