Alexey Serbin has posted comments on this change. Change subject: Add time/watermark based garbage collection to ResultTracker ......................................................................
Patch Set 15: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/3628/15//COMMIT_MSG Commit Message: Line 9: This adds time and watermark based garbage collection to the ResultTracker. Nit: the line is too long. Line 26: multithreaded threaded test that runs GC at the same time as writes. Nit: an extra 'threaded' 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 502: FLAGS_remember_responses_ttl_ms = 100; Is it OK that the modified flags are not returned back to their default values? May be, it's safer to add google::FlagSaver here? 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 flags. 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; Any concerns on backward compatibility here? If yes, does it make sense to add corresponding information into the release notes? http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: Line 104: RpcContext* ctx = new RpcContext(call, Wouldn't it be a memory leak if the state was one of COMPLETED, IN_PROGRESS or STALE? If yes, then please consider using unique_ptr<> wrapper around the newly allocated RpcContext instance. -- 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: 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
