Dan Burkert 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


PS15, Line 86:  
word missing


PS15, Line 95: For
Extra word?  Something is off in this sentence.


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 
request id by unique_ptr is pretty odd.  I can't figure out if it's because 
it's a heavyweight object, or because it's optional.  Looks to me like it 
should be ~40 bytes + whatever object overhead.


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 distinguish 
it from the other GC type.


PS15, Line 307: MustGcRecordFunc func
Ideally this would be a template param instead of a std::function so that it 
can get inlined (I imagine this is perf. sensitive since it's done under a 
spinlock).


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;
> RequestIDPB was only added during this release cycle, so no issue with olde
All of these fields should be optional, we shouldn't be introducing any more 
required proto fields.


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


-- 
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

Reply via email to