Todd Lipcon has posted comments on this change. Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: PS6, Line 627: ahm I think this comment would be more useful if it explained the intent a bit more - right now it's basically adding little value beyond the below if condition, which is pretty readable on its own. Maybe the comment could be 'Garbage collect rows that are deleted before the AHM' Also do we not have to worry about the case where the redo_head is a delete, but there are further reviews after that which are reinserts? Are we assuming that this method is only called after the REDOs have been collapsed such that there are no REINSERTS in it? In that case, I think a CHECK that (*redo_head)->next() == nullptr is probably a good idea, and documenting it in the header PS6, Line 679: : // Convert the redos into undos. Any redos that are eligible for history GC : // are applied and then thrown away (they don't generate undos). is this comment still relevant? Line 693: Mutation* current_undo; does this need to be defined up here anymore? only seeing it used in one scope, but maybe I missed something PS6, Line 699: \ nit: extra space Line 724: CHECK(redo_delete == nullptr); nit: CHECK_EQ PS6, Line 727: undo_encoder.SetToDelete(); this is old code, but why do we need to use undo_encoder here at all, vs just passing redo_mut.changelist() on line 730? Line 754: // Reset the UNDO head, losing all previous undos. we've lost the 'num_rows_history_truncated' thing now. I guess you're figuring that since this history truncation thing is going away very soon, you didn't want to bother maintaining in this patch? I'm cool with that, just wanted to confirm the thinking. http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: PS6, Line 175: Mutation** redo_head are the redos modified by this method? if so, should clarify in the doc. Otherwise, this should probably be a const Mutation* if we're just reading them PS6, Line 187: // 'history_gc_opts': Options indicating whether row history should be : // garbage-collected. no longer an argument -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
