Todd Lipcon has posted comments on this change.

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


Patch Set 16:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3076/16/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:

Line 215: 
some spurious whitespace added


PS16, Line 387: r
unused


http://gerrit.cloudera.org:8080/#/c/3076/16/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 958:             is_garbage_collected = true;
I don't think this is quite right, because you might hit a REINSERT delta here 
in the case of flushing an MRS.

EG if the MRS has a base row, plus a DELETE plus a REINSERT, and the DELETE is 
at a GCable timestamp, we still didn't actually GC the whole row. I think we 
need to keep running through the deltas until we see whether the _last_ one is 
a GCable DELETE

(would be good to add a test for this case too)


Line 1038:       if (is_garbage_collected) {
think this is better written: if (!is_garbage_collected) output_row_offset++;


http://gerrit.cloudera.org:8080/#/c/3076/16/src/kudu/tablet/tablet_history_gc-test.cc
File src/kudu/tablet/tablet_history_gc-test.cc:

Line 418:   // Delete every even row.
per elsewhere, I think this test would catch another bug if you do some of the 
deletes (and reinsert) while they're still in MRS.


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

Reply via email to