Todd Lipcon has posted comments on this change.

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

Patch Set 15:

Commit Message:

Line 9: This adds time and watermark based garbage collection to the 
> Nit: the line is too long.

Line 26: multithreaded threaded test that runs GC at the same time as writes.
> Nit: an extra 'threaded'
File src/kudu/rpc/

Line 502:   FLAGS_remember_responses_ttl_ms = 100;
> Is it OK that the modified flags are not returned back to their default val
we already wrap all of our tests in google::FlagSaver since the KuduTest base 
class has a FlagSaver member. I think some older tests that predate that 
addition have their own FlagSavers - could probably remove them now.

Line 540: // This test creates a thread continuously makes requests to the 
server, some lasting longer
> Nit: 'continuously makes' ---> 'continuously making'

Line 548:   FLAGS_remember_responses_ttl_ms = 10;
> Ditto: consider adding google::FlagSaver to restore default values for the 
File src/kudu/rpc/rpc_header.proto:

Line 126:   required int64 first_incomplete_seq_no = 3;
> Any concerns on backward compatibility here?  If yes, does it make sense to
RequestIDPB was only added during this release cycle, so no issue with older 
versions. The whole RequestIDPB itself is optional in the RPC header
File src/kudu/rpc/

Line 104:   RpcContext* ctx = new RpcContext(call,
> Wouldn't it be a memory leak if the state was one of COMPLETED, IN_PROGRESS
ah, good catch... I wonder why leak sanitizer builds are not catching this. 
Will investigate.

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

Reply via email to