Mike Percy has posted comments on this change. Change subject: KUDU-236. Implement tablet history GC ......................................................................
Patch Set 6: (11 comments) Posting partially addressed feedback except for 2 comments in compaction.cc and 2 comments in tablet_history_gc-test.cc http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.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 Fixed http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: Line 42: HistoryGcOpts(GcHistoryEnabled is_enabled, Timestamp ahm) > I don't think this ctor buys much over just using brace initialization like Done http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: Line 150: VLOG(2) << "Input Row: " << dst_row.schema()->DebugRow(dst_row) << > this should probably be a much higher VLOG level (and maybe DVLOG) Done Line 191: // TODO: Why keep the delete mutations? > because major delta compaction doesn't change row IDs, and thus you can't a Removed comment http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/tablet-harness.h File src/kudu/tablet/tablet-harness.h: 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. http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: 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 Done http://gerrit.cloudera.org:8080/#/c/3076/6/src/kudu/tablet/tablet_history_gc-test.cc File src/kudu/tablet/tablet_history_gc-test.cc: PS6, Line 73: ws > nit: capitalize RowSet Done 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 Done 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? Done > 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 matches. PS6, Line 169: No trace should remain after this > add an assertion that it actually didn't even generate a DRS Done -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master 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> Gerrit-HasComments: Yes
