Mike Percy has posted comments on this change.
Change subject: KUDU-236. Implement tablet history GC
Patch Set 6:
Posting partially addressed feedback except for 2 comments in compaction.cc and
2 comments in tablet_history_gc-test.cc
Line 835: num_rows_history_truncated += is_history_truncated;
> we lost the nice WARNING we used to have for this case which actually inclu
Line 42: HistoryGcOpts(GcHistoryEnabled is_enabled, Timestamp ahm)
> I don't think this ctor buys much over just using brace initialization like
Line 150: VLOG(2) << "Input Row: " << dst_row.schema()->DebugRow(dst_row)
> this should probably be a much higher VLOG level (and maybe DVLOG)
Line 191: // TODO: Why keep the delete mutations?
> because major delta compaction doesn't change row IDs, and thus you can't a
Line 112: clock_ = make_scoped_refptr(new server::HybridClock());
> do you need make_scoped_refptr here? given clock_ is a scoped_refptr I thin
Changed to reset() which is syntax that makes it more clear that we're giving
ownership to a smart ptr.
Line 111: DEFINE_int32(tablet_history_max_age_sec, 604800,
> can you express this as 60 * 60 * 24 * 7 so it's obvious it's 7 days?
I'll just set it to -1.
Line 114: "To disable history removal, set to -1.");
> I think we should probably disable this for the first commit, until we can
PS6, Line 73: ws
> nit: capitalize RowSet
PS6, Line 118: Major delta compaction will not remove UNDOs, so we cannot read
: // from before the initial insert of the data and expect to
see base data.
> not following this part of the comment -- you haven't run the MajorDeltaCom
Line 134: // equal to 2.
> I guess the comment above really applies here: you're saying that if you re
> I guess the comment above really applies here: you're saying that if you read
> from prior to the insertion, you'll still see the rows disappear because the
> UNDOs weren't removed, right?
> One suggestion to make these tests slightly easier to follow is to use
> RowSet::DebugDump() which gives you a nice printout per row that you can
> assert on. (though you'll have to set the hybrid timestamp to a constant
> instead of Now())
Spent some time implementing this idea, got some decent results with regex
PS6, Line 169: No trace should remain after this
> add an assertion that it actually didn't even generate a DRS
To view, visit http://gerrit.cloudera.org:8080/3076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>