David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT deltas in delta files ......................................................................
Patch Set 7: (10 comments) Abandoning this. Will review if I missed some comments tomorrow. http://gerrit.cloudera.org:8080/#/c/4819/7/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 657: Mutation* redo_head = nullptr; > I think this function would be a little easier to follow if we renamed this Done PS7, Line 709: // In the case where the previous undo was a nullptr just make this one : // the head. > this comment could be rephrased now that the if() below has been inverted refactored this code bit out (used multiple times). PS7, Line 711: NULL > nit: nullptr Done Line 725: // will have the timestamp of the DELETE REDO. > isn't this the undo encoder that is generated by the above? (this might dov see the new approach in the previous patch PS7, Line 735: Mutation* delete_redo = redo_head; : RowChangeListDecoder decoder(delete_redo->changelist()); : CHECK_OK(decoder.Init()); : DCHECK(decoder.is_delete()) << "Redo HEAD was not a delete: " : << delete_redo->changelist().ToString(*base_schema) << " " : << ERROR_LOG_CONTEXT; > maybe stick this in a {} block so that it's more obvious that the variables removed this and added a DCHECK(is_deleted). Together with the CHECK(redo_head != NULL) these should be good enough that we dont need the extra cruft PS7, Line 746: reinsert > I think this is creating a _delete_ undo, no? the inverse of a REINSERT is see previous patch. I refactored this. Line 751: if (history_gc_opts.IsAncientHistory(redo_mut->timestamp())) { > isn't it possible that only one of the resulting undos is GCable? it is: the delete could be gcable while the reinsert wasn't (the reverse cant happen as the reinsert must have a higher timestamp than the delete). However in this case we keep the delete all the same (which is why we always add both to the undo ll). This simplifies handling reinserts as we can always expect a delete before them. I don't think the extra delete in this rare case is a problem. PS7, Line 756: REINSERT > same Done Line 772: if (undo_head != NULL) { > undo_head is always non-null here, no? refactored this out PS7, Line 780: redo_head = Mutation::CreateInArena(arena, : redo_mut->timestamp(), : undo_encoder.as_changelist()); > should we have a CHECK here that redo_head == nullptr prior to this assignm Done -- To view, visit http://gerrit.cloudera.org:8080/4819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d421b8d2981a80c4485223b9b27a38d95e866a7 Gerrit-PatchSet: 7 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