From 8acaa8ef7d62ad13d0554edcd17839d8ae481d70 Mon Sep 17 00:00:00 2001 From: rishabhdaim Date: Wed, 4 Mar 2026 17:35:23 +0530 Subject: [PATCH 1/4] OAK-12119 : offline compaction does not persist compacted head into gc.log --- .../file/GarbageCollectionStrategy.java | 2 + .../oak/segment/file/GarbageCollector.java | 35 ++- ...SynchronizedGarbageCollectionStrategy.java | 7 + .../DefaultGarbageCollectionStrategyTest.java | 39 +++ ...GarbageCollectorOfflineCompactionTest.java | 257 ++++++++++++++++++ .../file/OfflineCompactionGcLogTest.java | 1 - 6 files changed, 337 insertions(+), 4 deletions(-) create mode 100644 oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectionStrategy.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectionStrategy.java index eba6c6e8248..5f757da972a 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectionStrategy.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectionStrategy.java @@ -100,4 +100,6 @@ interface Context { List cleanup(Context context) throws IOException; + List cleanup(Context context, CompactionResult compactionResult) throws IOException; + } diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java index a6db4207739..5c4c7764ec6 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java @@ -42,6 +42,7 @@ import org.apache.jackrabbit.oak.segment.file.tar.TarFiles; import org.apache.jackrabbit.oak.spi.blob.BlobStore; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; class GarbageCollector { @@ -112,6 +113,15 @@ class GarbageCollector { */ private SegmentGCOptions.GCType lastCompactionType = FULL; + /** + * Result of the last compaction, or {@code null} if no compaction has been + * performed since this store was opened or since the last cleanup. Used by + * standalone cleanup (e.g. offline compaction via oak-run) to persist the + * GC journal entry. + */ + @Nullable + private volatile CompactionResult lastCompactionResult; + private volatile boolean cancelRequested; GarbageCollector( @@ -292,17 +302,36 @@ synchronized void runTail(GarbageCollectionStrategy strategy) throws IOException synchronized CompactionResult compactFull(GarbageCollectionStrategy strategy) throws IOException { cancelRequested = false; - return strategy.compactFull(newGarbageCollectionContext(GC_COUNT.get())); + CompactionResult result = strategy.compactFull(newGarbageCollectionContext(GC_COUNT.get())); + if (result.requiresGCJournalEntry()) { + lastCompactionResult = result; + } + return result; } synchronized CompactionResult compactTail(GarbageCollectionStrategy strategy) throws IOException { cancelRequested = false; - return strategy.compactTail(newGarbageCollectionContext(GC_COUNT.get())); + CompactionResult result = strategy.compactTail(newGarbageCollectionContext(GC_COUNT.get())); + if (result.requiresGCJournalEntry()) { + lastCompactionResult = result; + } + return result; } synchronized List cleanup(GarbageCollectionStrategy strategy) throws IOException { cancelRequested = false; - return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get())); + CompactionResult compactionResult = lastCompactionResult; + lastCompactionResult = null; + if (compactionResult != null && compactionResult.requiresGCJournalEntry()) { + return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get()), compactionResult); + } + return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get()), CompactionResult.skipped( + lastCompactionType, + getGcGeneration(), + gcOptions, + revisionsSupplier.get().getHead(), + GC_COUNT.get() + )); } /** diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategy.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategy.java index f301b40c04a..9341ce6861c 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategy.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategy.java @@ -74,4 +74,11 @@ public List cleanup(Context context) throws IOException { } } + @Override + public List cleanup(Context context, CompactionResult compactionResult) throws IOException { + synchronized (lock) { + return strategy.cleanup(context, compactionResult); + } + } + } diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java index e0d4a2d7ae0..7dfe640c8c7 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java @@ -127,4 +127,43 @@ public void abortedCompactionDoesNotPersistToJournal() throws Exception { runCleanup(CompactionResult.aborted(GCGeneration.NULL, 0)); verifyGCJournalPersistence(never()); } + + @Test + public void offlineCompactionAfterSuccessfulFullCompactionPersistsToJournal() throws Exception { + CompactionResult result = CompactionResult.succeeded( + SegmentGCOptions.GCType.FULL, + GCGeneration.NULL, + SegmentGCOptions.defaultGCOptions(), + RecordId.NULL, + 0); + runCleanup(result); + verifyGCJournalPersistence(times(1)); + } + + @Test + public void offlineCompactionAfterSuccessfulTailCompactionPersistsToJournal() throws Exception { + CompactionResult result = CompactionResult.succeeded( + SegmentGCOptions.GCType.TAIL, + GCGeneration.NULL, + SegmentGCOptions.defaultGCOptions(), + RecordId.NULL, + 0); + runCleanup(result); + verifyGCJournalPersistence(times(1)); + } + + @Test + public void abortedRetryDoesNotOverwritePriorSucceededResultForJournalPersistence() throws Exception { + // Simulates: compactFull -> aborted, compactFull -> succeeded, cleanup. + // GarbageCollector stores only the succeeded result (isSuccess() gate), + // so cleanup is ultimately called with the succeeded result. + runCleanup(CompactionResult.aborted(GCGeneration.NULL, 0)); + runCleanup(CompactionResult.succeeded( + SegmentGCOptions.GCType.FULL, + GCGeneration.NULL, + SegmentGCOptions.defaultGCOptions(), + RecordId.NULL, + 0)); + verifyGCJournalPersistence(times(1)); + } } diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java new file mode 100644 index 00000000000..e505c3ea824 --- /dev/null +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java @@ -0,0 +1,257 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.segment.file; + +import org.apache.jackrabbit.oak.segment.RecordId; +import org.apache.jackrabbit.oak.segment.compaction.SegmentGCOptions; +import org.apache.jackrabbit.oak.segment.file.cancel.Canceller; +import org.apache.jackrabbit.oak.segment.spi.persistence.GCGeneration; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Verifies the {@code lastCompactionResult} lifecycle inside {@link GarbageCollector} + * for the offline compaction path, covering all combinations of {@code compactFull()} + * and {@code cleanup()} calls described in the scenario matrix: + * + * 
+ * compact(ok) → cleanup                    journal written
+ * compact(ok) → compact(ok) → cleanup      gen2 supersedes gen1
+ * compact(ok) → compact(abort) → cleanup   abort does not clobber gen1
+ * compact(abort) → compact(ok) → cleanup   journal written for gen2
+ * compact(abort) → cleanup                 journal NOT written
+ * cleanup → compact(ok) → cleanup          journal written only for second cleanup
+ * compact(ok) → cleanup → cleanup          second cleanup is a no-op
+ *
+ * + *

The strategy is mocked so that specific {@link CompactionResult} instances can be + * injected. {@link ArgumentCaptor} is then used to assert exactly which result was + * forwarded to {@code strategy.cleanup()}, proving that the 2-arg + * {@code cleanup(Context, CompactionResult)} path (journal entry required) vs the 1-arg + * {@code cleanup(Context)} path (no journal entry) is chosen correctly. + */ +public class GarbageCollectorOfflineCompactionTest { + + private GarbageCollectionStrategy strategy; + private GarbageCollector collector; + + @Before + public void setUp() { + strategy = Mockito.mock(GarbageCollectionStrategy.class); + collector = new GarbageCollector( + SegmentGCOptions.defaultGCOptions(), + Mockito.mock(GCListener.class), + null, // gcJournal — not needed; strategy is mocked + new AtomicBoolean(true), + null, // fileReaper + null, // tarFiles + null, // tracker + null, // segmentReader + () -> null, // revisionsSupplier + null, // blobStore + null, // segmentCache + null, // segmentWriter + null, // stats + Canceller.newCanceller(), + () -> {}, // flusher + null // segmentWriterFactory + ); + } + + private CompactionResult succeeded(int gen) { + return CompactionResult.succeeded( + SegmentGCOptions.GCType.FULL, + GCGeneration.newGCGeneration(gen, gen, false), + SegmentGCOptions.defaultGCOptions(), + RecordId.NULL, + gen + ); + } + + private CompactionResult aborted(int gen) { + return CompactionResult.aborted(GCGeneration.newGCGeneration(gen, gen, false), gen); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(ok) → cleanup + // The succeeded result must be passed to the 2-arg cleanup. + // ----------------------------------------------------------------------- + + @Test + public void testCompactOk_cleanup_journalWritten() throws IOException { + CompactionResult result = succeeded(1); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(result); + + collector.compactFull(strategy); + collector.cleanup(strategy); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy).cleanup( + Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame(result, captor.getValue()); + Assert.assertTrue(captor.getValue().requiresGCJournalEntry()); + Mockito.verify(strategy, Mockito.never()) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class)); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(ok) → compact(ok) → cleanup + // The second succeeded result supersedes the first. + // ----------------------------------------------------------------------- + + @Test + public void testCompactOk_compactOk_cleanup_gen2Supersedes() throws IOException { + CompactionResult gen1 = succeeded(1); + CompactionResult gen2 = succeeded(2); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(gen1) + .thenReturn(gen2); + + collector.compactFull(strategy); + collector.compactFull(strategy); + collector.cleanup(strategy); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy).cleanup( + Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame("gen2 must supersede gen1", gen2, captor.getValue()); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(ok) → compact(abort) → cleanup + // An aborted result must not overwrite the previous succeeded result. + // ----------------------------------------------------------------------- + + @Test + public void testCompactOk_compactAbort_cleanup_abortDoesNotClobber() throws IOException { + CompactionResult gen1 = succeeded(1); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(gen1) + .thenReturn(aborted(1)); + + collector.compactFull(strategy); + collector.compactFull(strategy); + collector.cleanup(strategy); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy).cleanup( + Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame("abort must not clobber the gen1 succeeded result", gen1, captor.getValue()); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(abort) → compact(ok) → cleanup + // The initial abort must not prevent a subsequent succeeded result from + // being used in cleanup. + // ----------------------------------------------------------------------- + + @Test + public void testCompactAbort_compactOk_cleanup_journalWritten() throws IOException { + CompactionResult gen2 = succeeded(2); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(aborted(0)) + .thenReturn(gen2); + + collector.compactFull(strategy); + collector.compactFull(strategy); + collector.cleanup(strategy); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy).cleanup( + Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame(gen2, captor.getValue()); + Assert.assertTrue(captor.getValue().requiresGCJournalEntry()); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(abort) → cleanup + // No succeeded result is available; cleanup must use the 1-arg (skipped) + // path — no journal entry. + // ----------------------------------------------------------------------- + + @Test + public void testCompactAbort_cleanup_noJournalEntry() throws IOException { + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(aborted(0)); + + collector.compactFull(strategy); + collector.cleanup(strategy); + + Mockito.verify(strategy) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class)); + Mockito.verify(strategy, Mockito.never()) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class), + Mockito.any(CompactionResult.class)); + } + + // ----------------------------------------------------------------------- + // Scenario: cleanup → compact(ok) → cleanup + // The first (pre-compaction) cleanup must not write a journal entry; + // only the second cleanup (after a succeeded compaction) must. + // ----------------------------------------------------------------------- + + @Test + public void testCleanup_compactOk_cleanup_journalWrittenOnlyForSecond() throws IOException { + CompactionResult result = succeeded(1); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(result); + + collector.cleanup(strategy); // no preceding compact — 1-arg path + collector.compactFull(strategy); + collector.cleanup(strategy); // succeeded result available — 2-arg path + + Mockito.verify(strategy, Mockito.times(1)) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class)); + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy, Mockito.times(1)) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame(result, captor.getValue()); + } + + // ----------------------------------------------------------------------- + // Scenario: compact(ok) → cleanup → cleanup + // The first cleanup consumes lastCompactionResult; the second must fall + // back to the 1-arg (skipped) path — no duplicate journal entry. + // ----------------------------------------------------------------------- + + @Test + public void testCompactOk_cleanup_cleanup_secondCleanupIsNoop() throws IOException { + CompactionResult result = succeeded(1); + Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(result); + + collector.compactFull(strategy); + collector.cleanup(strategy); // 2-arg path; clears lastCompactionResult + collector.cleanup(strategy); // 1-arg path; lastCompactionResult is null + + Mockito.verify(strategy, Mockito.times(1)) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class), + Mockito.any(CompactionResult.class)); + Mockito.verify(strategy, Mockito.times(1)) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class)); + } + +} diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java index a344df74199..e99560eec64 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java @@ -43,7 +43,6 @@ * and pass once the fix that retains the {@code CompactionResult} across the two calls * is applied. */ -@Ignore public class OfflineCompactionGcLogTest { @Rule From a9e7ac639566d9f869f833175e1d51234658bf92 Mon Sep 17 00:00:00 2001 From: rishabhdaim Date: Wed, 4 Mar 2026 23:26:29 +0530 Subject: [PATCH 2/4] OAK-12119 : fix cleanup fallback path and rename tests to camelCase --- .../oak/segment/file/GarbageCollector.java | 15 +++++---------- .../GarbageCollectorOfflineCompactionTest.java | 16 ++++++++-------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java index 5c4c7764ec6..14666442671 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java @@ -321,17 +321,12 @@ synchronized CompactionResult compactTail(GarbageCollectionStrategy strategy) th synchronized List cleanup(GarbageCollectionStrategy strategy) throws IOException { cancelRequested = false; CompactionResult compactionResult = lastCompactionResult; - lastCompactionResult = null; - if (compactionResult != null && compactionResult.requiresGCJournalEntry()) { - return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get()), compactionResult); + if (compactionResult != null) { + List result = strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get()), compactionResult); + lastCompactionResult = null; + return result; } - return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get()), CompactionResult.skipped( - lastCompactionType, - getGcGeneration(), - gcOptions, - revisionsSupplier.get().getHead(), - GC_COUNT.get() - )); + return strategy.cleanup(newGarbageCollectionContext(GC_COUNT.get())); } /** diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java index e505c3ea824..6fcb4eb342c 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java @@ -58,7 +58,7 @@ public class GarbageCollectorOfflineCompactionTest { private GarbageCollector collector; @Before - public void setUp() { + public void setUp() throws IOException { strategy = Mockito.mock(GarbageCollectionStrategy.class); collector = new GarbageCollector( SegmentGCOptions.defaultGCOptions(), @@ -100,7 +100,7 @@ private CompactionResult aborted(int gen) { // ----------------------------------------------------------------------- @Test - public void testCompactOk_cleanup_journalWritten() throws IOException { + public void testCompactOkCleanupJournalWritten() throws IOException { CompactionResult result = succeeded(1); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(result); @@ -123,7 +123,7 @@ public void testCompactOk_cleanup_journalWritten() throws IOException { // ----------------------------------------------------------------------- @Test - public void testCompactOk_compactOk_cleanup_gen2Supersedes() throws IOException { + public void testCompactOkCompactOkCleanupGen2Supersedes() throws IOException { CompactionResult gen1 = succeeded(1); CompactionResult gen2 = succeeded(2); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) @@ -146,7 +146,7 @@ public void testCompactOk_compactOk_cleanup_gen2Supersedes() throws IOException // ----------------------------------------------------------------------- @Test - public void testCompactOk_compactAbort_cleanup_abortDoesNotClobber() throws IOException { + public void testCompactOkCompactAbortCleanupAbortDoesNotClobber() throws IOException { CompactionResult gen1 = succeeded(1); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(gen1) @@ -169,7 +169,7 @@ public void testCompactOk_compactAbort_cleanup_abortDoesNotClobber() throws IOEx // ----------------------------------------------------------------------- @Test - public void testCompactAbort_compactOk_cleanup_journalWritten() throws IOException { + public void testCompactAbortCompactOkCleanupJournalWritten() throws IOException { CompactionResult gen2 = succeeded(2); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(aborted(0)) @@ -193,7 +193,7 @@ public void testCompactAbort_compactOk_cleanup_journalWritten() throws IOExcepti // ----------------------------------------------------------------------- @Test - public void testCompactAbort_cleanup_noJournalEntry() throws IOException { + public void testCompactAbortCleanupNoJournalEntry() throws IOException { Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(aborted(0)); @@ -214,7 +214,7 @@ public void testCompactAbort_cleanup_noJournalEntry() throws IOException { // ----------------------------------------------------------------------- @Test - public void testCleanup_compactOk_cleanup_journalWrittenOnlyForSecond() throws IOException { + public void testCleanupCompactOkCleanupJournalWrittenOnlyForSecond() throws IOException { CompactionResult result = succeeded(1); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(result); @@ -238,7 +238,7 @@ public void testCleanup_compactOk_cleanup_journalWrittenOnlyForSecond() throws I // ----------------------------------------------------------------------- @Test - public void testCompactOk_cleanup_cleanup_secondCleanupIsNoop() throws IOException { + public void testCompactOkCleanupCleanupSecondCleanupIsNoop() throws IOException { CompactionResult result = succeeded(1); Mockito.when(strategy.compactFull(Mockito.any(GarbageCollectionStrategy.Context.class))) .thenReturn(result); From 01f6193d8db9fb88e8b623270590652a97b8200c Mon Sep 17 00:00:00 2001 From: rishabhdaim Date: Thu, 5 Mar 2026 01:32:02 +0530 Subject: [PATCH 3/4] OAK-12119 : improve test coverage and fix static imports --- .../DefaultGarbageCollectionStrategyTest.java | 70 +++++--------- ...GarbageCollectorOfflineCompactionTest.java | 24 +++++ .../file/OfflineCompactionGcLogTest.java | 1 - ...hronizedGarbageCollectionStrategyTest.java | 93 +++++++++++++++++++ 4 files changed, 142 insertions(+), 46 deletions(-) create mode 100644 oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategyTest.java diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java index 7dfe640c8c7..20b92dba702 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/DefaultGarbageCollectionStrategyTest.java @@ -23,48 +23,40 @@ import org.apache.jackrabbit.oak.segment.SegmentTracker; import org.apache.jackrabbit.oak.segment.compaction.SegmentGCOptions; import org.apache.jackrabbit.oak.segment.file.tar.CleanupContext; -import org.apache.jackrabbit.oak.segment.spi.persistence.GCGeneration; import org.apache.jackrabbit.oak.segment.file.tar.TarFiles; import org.apache.jackrabbit.oak.segment.memory.MemoryStore; +import org.apache.jackrabbit.oak.segment.spi.persistence.GCGeneration; import org.junit.Test; import org.mockito.Mockito; import org.mockito.verification.VerificationMode; import java.io.IOException; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - public class DefaultGarbageCollectionStrategyTest { private final GCJournal journal; public DefaultGarbageCollectionStrategyTest() { journal = Mockito.mock(GCJournal.class); - when(journal.read()).thenReturn(Mockito.mock(GCJournal.GCJournalEntry.class)); + Mockito.when(journal.read()).thenReturn(Mockito.mock(GCJournal.GCJournalEntry.class)); } private GarbageCollectionStrategy.Context getMockedGCContext(MemoryStore store) throws IOException { GarbageCollectionStrategy.Context mockedContext = Mockito.mock(GarbageCollectionStrategy.Context.class); - when(mockedContext.getGCListener()).thenReturn(Mockito.mock(GCListener.class)); - when(mockedContext.getTarFiles()).thenReturn(Mockito.mock(TarFiles.class)); - when(mockedContext.getSegmentCache()).thenReturn(Mockito.mock(SegmentCache.class)); - when(mockedContext.getFileStoreStats()).thenReturn(Mockito.mock(FileStoreStats.class)); + Mockito.when(mockedContext.getGCListener()).thenReturn(Mockito.mock(GCListener.class)); + Mockito.when(mockedContext.getTarFiles()).thenReturn(Mockito.mock(TarFiles.class)); + Mockito.when(mockedContext.getSegmentCache()).thenReturn(Mockito.mock(SegmentCache.class)); + Mockito.when(mockedContext.getFileStoreStats()).thenReturn(Mockito.mock(FileStoreStats.class)); SegmentTracker tracker = new SegmentTracker((msb, lsb) -> new SegmentId(store, msb, lsb)); - when(mockedContext.getSegmentTracker()).thenReturn(tracker); - when(mockedContext.getCompactionMonitor()).thenReturn(GCNodeWriteMonitor.EMPTY); - when(mockedContext.getRevisions()).thenReturn(store.getRevisions()); - when(mockedContext.getGCJournal()).thenReturn(journal); + Mockito.when(mockedContext.getSegmentTracker()).thenReturn(tracker); + Mockito.when(mockedContext.getCompactionMonitor()).thenReturn(GCNodeWriteMonitor.EMPTY); + Mockito.when(mockedContext.getRevisions()).thenReturn(store.getRevisions()); + Mockito.when(mockedContext.getGCJournal()).thenReturn(journal); TarFiles mockedTarFiles = Mockito.mock(TarFiles.class); - when(mockedContext.getTarFiles()).thenReturn(mockedTarFiles); - when(mockedTarFiles.cleanup(any(CleanupContext.class))) + Mockito.when(mockedContext.getTarFiles()).thenReturn(mockedTarFiles); + Mockito.when(mockedTarFiles.cleanup(Mockito.any(CleanupContext.class))) .thenReturn(Mockito.mock(TarFiles.CleanupResult.class)); return mockedContext; @@ -77,12 +69,12 @@ private void runCleanup(CompactionResult result) throws IOException { } private void verifyGCJournalPersistence(VerificationMode mode) { - verify(journal, mode).persist( - anyLong(), - anyLong(), - any(GCGeneration.class), - anyLong(), - anyString()); + Mockito.verify(journal, mode).persist( + Mockito.anyLong(), + Mockito.anyLong(), + Mockito.any(GCGeneration.class), + Mockito.anyLong(), + Mockito.anyString()); } @Test @@ -94,14 +86,14 @@ public void successfulCompactionPersistsToJournal() throws Exception { RecordId.NULL, 0); runCleanup(result); - verifyGCJournalPersistence(times(1)); + verifyGCJournalPersistence(Mockito.times(1)); } @Test public void partialCompactionDoesNotPersistToJournal() throws Exception { CompactionResult result = CompactionResult.partiallySucceeded(GCGeneration.NULL, RecordId.NULL, 0); runCleanup(result); - verifyGCJournalPersistence(never()); + verifyGCJournalPersistence(Mockito.never()); } @Test @@ -113,31 +105,19 @@ public void skippedCompactionDoesNotPersistToJournal() throws Exception { RecordId.NULL, 0); runCleanup(result); - verifyGCJournalPersistence(never()); + verifyGCJournalPersistence(Mockito.never()); } @Test public void nonApplicableCompactionDoesNotPersistToJournal() throws Exception { runCleanup(CompactionResult.notApplicable(0)); - verifyGCJournalPersistence(never()); + verifyGCJournalPersistence(Mockito.never()); } @Test public void abortedCompactionDoesNotPersistToJournal() throws Exception { runCleanup(CompactionResult.aborted(GCGeneration.NULL, 0)); - verifyGCJournalPersistence(never()); - } - - @Test - public void offlineCompactionAfterSuccessfulFullCompactionPersistsToJournal() throws Exception { - CompactionResult result = CompactionResult.succeeded( - SegmentGCOptions.GCType.FULL, - GCGeneration.NULL, - SegmentGCOptions.defaultGCOptions(), - RecordId.NULL, - 0); - runCleanup(result); - verifyGCJournalPersistence(times(1)); + verifyGCJournalPersistence(Mockito.never()); } @Test @@ -149,7 +129,7 @@ public void offlineCompactionAfterSuccessfulTailCompactionPersistsToJournal() th RecordId.NULL, 0); runCleanup(result); - verifyGCJournalPersistence(times(1)); + verifyGCJournalPersistence(Mockito.times(1)); } @Test @@ -164,6 +144,6 @@ public void abortedRetryDoesNotOverwritePriorSucceededResultForJournalPersistenc SegmentGCOptions.defaultGCOptions(), RecordId.NULL, 0)); - verifyGCJournalPersistence(times(1)); + verifyGCJournalPersistence(Mockito.times(1)); } } diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java index 6fcb4eb342c..2828a74587d 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java @@ -94,6 +94,30 @@ private CompactionResult aborted(int gen) { return CompactionResult.aborted(GCGeneration.newGCGeneration(gen, gen, false), gen); } + // ----------------------------------------------------------------------- + // Scenario: compactTail(ok) → cleanup + // The compactTail path stores lastCompactionResult identically to + // compactFull; the succeeded result must reach the 2-arg cleanup. + // ----------------------------------------------------------------------- + + @Test + public void testCompactTailOkCleanupJournalWritten() throws IOException { + CompactionResult result = succeeded(1); + Mockito.when(strategy.compactTail(Mockito.any(GarbageCollectionStrategy.Context.class))) + .thenReturn(result); + + collector.compactTail(strategy); + collector.cleanup(strategy); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CompactionResult.class); + Mockito.verify(strategy).cleanup( + Mockito.any(GarbageCollectionStrategy.Context.class), captor.capture()); + Assert.assertSame(result, captor.getValue()); + Assert.assertTrue(captor.getValue().requiresGCJournalEntry()); + Mockito.verify(strategy, Mockito.never()) + .cleanup(Mockito.any(GarbageCollectionStrategy.Context.class)); + } + // ----------------------------------------------------------------------- // Scenario: compact(ok) → cleanup // The succeeded result must be passed to the 2-arg cleanup. diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java index e99560eec64..3f13bc11bcb 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/OfflineCompactionGcLogTest.java @@ -24,7 +24,6 @@ import org.apache.jackrabbit.oak.segment.file.tar.TarPersistence; import org.apache.jackrabbit.oak.segment.spi.persistence.GCGeneration; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategyTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategyTest.java new file mode 100644 index 00000000000..26ce1ae50d8 --- /dev/null +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/SynchronizedGarbageCollectionStrategyTest.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.segment.file; + +import org.apache.jackrabbit.oak.segment.RecordId; +import org.apache.jackrabbit.oak.segment.compaction.SegmentGCOptions; +import org.apache.jackrabbit.oak.segment.spi.persistence.GCGeneration; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import java.io.IOException; +import java.util.Collections; + +/** + * Verifies that {@link SynchronizedGarbageCollectionStrategy} correctly + * delegates all method calls to the wrapped {@link GarbageCollectionStrategy}. + */ +public class SynchronizedGarbageCollectionStrategyTest { + + private GarbageCollectionStrategy delegate; + private SynchronizedGarbageCollectionStrategy strategy; + + @Before + public void setUp() { + delegate = Mockito.mock(GarbageCollectionStrategy.class); + strategy = new SynchronizedGarbageCollectionStrategy(delegate); + } + + @Test + public void testCleanupWithCompactionResultDelegatesToWrappedStrategy() throws IOException { + GarbageCollectionStrategy.Context context = Mockito.mock(GarbageCollectionStrategy.Context.class); + CompactionResult compactionResult = CompactionResult.succeeded( + SegmentGCOptions.GCType.FULL, + GCGeneration.NULL, + SegmentGCOptions.defaultGCOptions(), + RecordId.NULL, + 0); + Mockito.when(delegate.cleanup(context, compactionResult)).thenReturn(Collections.emptyList()); + + strategy.cleanup(context, compactionResult); + + Mockito.verify(delegate).cleanup(context, compactionResult); + } + + @Test + public void testCleanupDelegatesToWrappedStrategy() throws IOException { + GarbageCollectionStrategy.Context context = Mockito.mock(GarbageCollectionStrategy.Context.class); + Mockito.when(delegate.cleanup(context)).thenReturn(Collections.emptyList()); + + strategy.cleanup(context); + + Mockito.verify(delegate).cleanup(context); + } + + @Test + public void testCompactFullDelegatesToWrappedStrategy() throws IOException { + GarbageCollectionStrategy.Context context = Mockito.mock(GarbageCollectionStrategy.Context.class); + CompactionResult expected = CompactionResult.aborted(GCGeneration.NULL, 0); + Mockito.when(delegate.compactFull(context)).thenReturn(expected); + + CompactionResult actual = strategy.compactFull(context); + + Mockito.verify(delegate).compactFull(context); + org.junit.Assert.assertSame(expected, actual); + } + + @Test + public void testCompactTailDelegatesToWrappedStrategy() throws IOException { + GarbageCollectionStrategy.Context context = Mockito.mock(GarbageCollectionStrategy.Context.class); + CompactionResult expected = CompactionResult.aborted(GCGeneration.NULL, 0); + Mockito.when(delegate.compactTail(context)).thenReturn(expected); + + CompactionResult actual = strategy.compactTail(context); + + Mockito.verify(delegate).compactTail(context); + org.junit.Assert.assertSame(expected, actual); + } +} From 36356891f1fdfea5ef481c5a7f154bf5eeea8a56 Mon Sep 17 00:00:00 2001 From: rishabhdaim Date: Thu, 5 Mar 2026 12:47:58 +0530 Subject: [PATCH 4/4] OAK-12119 : remove volatile from lastCompactionResult and fix test setup --- .../apache/jackrabbit/oak/segment/file/GarbageCollector.java | 2 +- .../segment/file/GarbageCollectorOfflineCompactionTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java index 14666442671..af80e3f6f50 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GarbageCollector.java @@ -120,7 +120,7 @@ class GarbageCollector { * GC journal entry. */ @Nullable - private volatile CompactionResult lastCompactionResult; + private CompactionResult lastCompactionResult; private volatile boolean cancelRequested; diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java index 2828a74587d..56292546065 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GarbageCollectorOfflineCompactionTest.java @@ -58,7 +58,7 @@ public class GarbageCollectorOfflineCompactionTest { private GarbageCollector collector; @Before - public void setUp() throws IOException { + public void setUp() { strategy = Mockito.mock(GarbageCollectionStrategy.class); collector = new GarbageCollector( SegmentGCOptions.defaultGCOptions(), @@ -233,7 +233,7 @@ public void testCompactAbortCleanupNoJournalEntry() throws IOException { // ----------------------------------------------------------------------- // Scenario: cleanup → compact(ok) → cleanup - // The first (pre-compaction) cleanup must not write a journal entry; + // The first (pre-compaction) cleanup must not write a journal entry // only the second cleanup (after a succeeded compaction) must. // -----------------------------------------------------------------------