[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT deltas in delta files

["David Ribeiro Alves (Code Review)" <ge...@cloudera.org>]

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 <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes