Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20546 )
Change subject: [compaction] Skip memory allocation for ancient undo deltas ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20546/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20546/1//COMMIT_MSG@7 PS1, Line 7: [compaction] Skip memory allocation for ancient undo deltas > I think it's necessary to add a test at least to verify that the data is st Thanks for the review, Alexey. Apologies for being late on this. There is one test (TestNoGenerateUndoOnMajorDeltaCompaction) that does cover this code path already. It updates rows couple of times and then flushes the DMS thereby creating REDO deltas for those updates. Later, it moves the clock forwards to make those deltas ancient. Subsequent reads on the rows are verified for both the updates. Lastly, major delta compaction is run that results in this execution of exactly this piece of code where REDO mutations are processed and converted to UNDO deltas. However, in this specific case, we end with AHM case because by now all these deltas are ancient. So, what we have eventually is this: Base row data filled with latest update and no REDO mutations and just one UNDO delta for DELETE for first insert op. The only difference here is that REDO was applied from major delta compaction instead of rowset compaction. Since ApplyMutationsAndGenerateUndos is applicable to both Major and RowSet compaction, it doesn't make much difference except that major compaction won't touch the already generated UNDO deltas as it only works on currently existing REDO deltas and applies those to base row. However, rowset compaction does take into consideration existing UNDO deltas which in this specific test case would be only 1 in number (for the first insert). Either way, we are only working on the existing REDO deltas here and make sure that we don't allocate memory for their corresponding in-memory UNDO delta object if the REDO mutation is ancient. So, in a nutshell, the existing test already verifies that there is no UNDO delta created for ancient REDO mutations and also make sure that those REDO updates are applied to the base row thereby verifying that correct data is applied on the row. I hope my explanation above didn't end up creating more confusion :-) If you want, I can add one similar test that does the similar steps and verify at the end the following: 1. There is no ancient UNDO delta as well as REDO delta present. 2. The row has latest value applied to it. If you have something different and extra in mind with regard to fuzz tests, I am open to discussion. -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Thu, 19 Oct 2023 11:51:05 +0000 Gerrit-HasComments: Yes
