David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts ......................................................................
Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/4791/5/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 728: // row but disregard the undo reinsert that is created. > What if we added a 'bool* row_exists' out-param to ApplyRowChanges, so it t But in that case we would be generating a REINSERT UNDO for all deletes, which I was trying to avoid and would likely be unnecessary in large majority of cases we see a delete (as you know REINSERTS are the biggest and most expensive UNDOs to build). Plus we'd have to keep that "on the side" just in case we needed it for the following mutation and when we did see a REINSERT we'd go through row again. This seems a steep price to pay for the advantage of having a single method that returns an UNDO that matches precisely the encoded REDO. I think a bit of this problem stems from our naming choices, after all ApplyRowChanges feels like it should return an UNDO because of the argument naming choice. What if we broke that connection and just returned an 'out' like in RemoveColumnIdsFromChangeList. We could also change the name of the method to something like MutateRowAndCaptureChanges. With the "undo" connection broken the purpose of the method is clearer. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/4791 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 Gerrit-PatchSet: 5 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
