David Ribeiro Alves 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 Done 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? Done Line 693: Mutation* current_undo; > does this need to be defined up here anymore? only seeing it used in one sc yeah, in a follow up I also use in the reinsert block, mind if I keep it here until then? PS6, Line 699: \ > nit: extra space Done Line 724: CHECK(redo_delete == nullptr); > nit: CHECK_EQ The compiler complains about this: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const nullptr_t') Is this something we usually do or am I missing something. PS6, Line 727: undo_encoder.SetToDelete(); > this is old code, but why do we need to use undo_encoder here at all, vs ju we don't, but this will change in a follow up patch so I'd rather leave it and fix it there, if thats ok Line 754: // Reset the UNDO head, losing all previous undos. > we've lost the 'num_rows_history_truncated' thing now. I guess you're figur exactly. 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. Ot Done PS6, Line 187: // 'history_gc_opts': Options indicating whether row history should be : // garbage-collected. > no longer an argument Done -- 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 <dral...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
