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

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

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

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)

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

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.

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 

File src/kudu/tablet/tablet_history_gc-test.cc:

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

Reply via email to