Todd Lipcon has posted comments on this change. Change subject: KUDU-236 (part 1). Implement tablet history GC ......................................................................
Patch Set 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/3076/20/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: 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())) { same 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 http://gerrit.cloudera.org:8080/#/c/3076/20/src/kudu/tablet/compaction.h 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: https://www.chromium.org/rvalue-references (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 http://gerrit.cloudera.org:8080/#/c/3076/20/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1535: history_gc_opts.IsAncientHistory(*snap_timestamp)) { same -- To view, visit http://gerrit.cloudera.org:8080/3076 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If9833a863f118eb82be80ea56204d0d9141611c2 Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
