Todd Lipcon has posted comments on this change.

Change subject: KUDU-236 (part 1). Implement tablet history GC

Patch Set 20:

File src/kudu/tablet/

Line 599:   if (history_gc_opts.gc_enabled()) {
style: do an early return here instead of indenting the whole function

PS20, Line 699: history_gc_opts.gc_enabled() && 
now that the 'disabled' GC opts actually sets the ancient history mark to 0, I 
think the 'enabled' check is a redundant branch. (ie IsAncientHistory already 
returns false if it's disabled)

Line 765:             history_gc_opts.IsAncientHistory(redo_mut->timestamp())) {

Line 799:   rowid_t input_row_offset = -1;
hrm, your comment says 'removed' but still see this

Line 804:     int num_rows = rows.size();
why this extra variable here? why not just use rows.size() in the loop before? 
with optimizations on i'm pretty sure the compiler's smart enough to hoist it

Line 954:               history_gc_opts.IsAncientHistory(mut->timestamp())) {
and here
File src/kudu/tablet/compaction.h:

PS20, Line 39: std::move(ahm
not a big deal (no need to rev the patch for it) but I don't really like the 
use of std::move() for objects that are just POD and don't have a special move 
constructor... it's clearly not a performance gain here and I don't think it 
makes the code clearer (rather it makes it seem like something fancy might be 
going on)

The GSG is kind of quiet on move, but the Chromium style guide seems to agree 
with the above: (see point 10)

PS20, Line 63:   enum GcHistoryEnabled {
             :     GC_DISABLED = 0,
             :     GC_ENABLED = 1,
             :   };
now that this is a private detail, let's just use bool? not adding a lot since 
it's such a small class and the 'enabled_' name of the bool makes it obvious
File src/kudu/tserver/

Line 1535:       history_gc_opts.IsAncientHistory(*snap_timestamp)) {

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If9833a863f118eb82be80ea56204d0d9141611c2
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to