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 > Thank you for putting together the explanation above, iterating over the sa Yes, both #1 and #2 are covered under the newly added test. As for #3, it is partially covered as well where the test makes sure that scan on old timestamps return the expected data for both cases i.e. Before compaction: scan@ts[after_insert] -> 0 scan@ts[after_update1] -> 1 scan@ts[after_update2] -> 2 After compaction, scan ops return 2 because there is no UNDO delta present anymore so the only data changes that are present are the latest one: scan@ts[after_insert] -> 2 scan@ts[after_update1] -> 2 scan@ts[after_update2] -> 2 Additionally, there is one existing test i.e. TestGhostRowsNotRevived that creates ghost rows, turns them ancient and ensures those are not to be found after compaction job. I thought about adding metrics but IMO it turns out to be an overkill just to get the information on how many times memory allocation was skipped. It will require passing of tablet object's metrics to standalone methods like FlushCompactionInput, MergeDuplicatedRowHistory, ApplyMutationsAndGenerateUndos along with changing the callers everywhere including flush operation workflows. I would rather not add metrics. FWIW, I did add metrics code in the test and verified that it was working fine. Excerpts: >>> // Assert that there was no memory allocation for UNDO delta for corresponding ancient REDOs int expected_total_mutations = kNumRowsets * rows_per_rowset_ * 2; ASSERT_EQ(expected_total_mutations, metrics->ancient_redo_compact_mem_alloc_skipped->value()); --- if (history_gc_opts.IsAncientHistory(redo_mut->timestamp())) { if (metrics) metrics->ancient_redo_compact_mem_alloc_skipped->Increment(); continue; } <<< Let me see if there is something else on the lines of multiple ghosts spread across a AHM mark that needs to be covered. -- 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: Tue, 24 Oct 2023 15:51:24 +0000 Gerrit-HasComments: Yes
