This is an automated email from the ASF dual-hosted git repository. daim pushed a commit to branch OAK-12119 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 8acaa8ef7d62ad13d0554edcd17839d8ae481d70 Author: rishabhdaim <[email protected]> AuthorDate: Wed Mar 4 17:35:23 2026 +0530 OAK-12119 : offline compaction does not persist compacted head into gc.log --- .../segment/file/GarbageCollectionStrategy.java | 2 + .../oak/segment/file/GarbageCollector.java | 35 ++- .../SynchronizedGarbageCollectionStrategy.java | 7 + .../file/DefaultGarbageCollectionStrategyTest.java | 39 ++++ .../GarbageCollectorOfflineCompactionTest.java | 257 +++++++++++++++++++++ .../segment/file/OfflineCompactionGcLogTest.java | 1 - 6 files changed, 337 insertions(+), 4 deletions(-) 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 eba6c6e824..5f757da972 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 GarbageCollectionStrategy { List<String> cleanup(Context context) throws IOException; + List<String> 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 a6db420773..5c4c7764ec 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.spi.persistence.GCGeneration; 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 @@ class GarbageCollector { 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<String> 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 f301b40c04..9341ce6861 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 @@ class SynchronizedGarbageCollectionStrategy implements GarbageCollectionStrategy } } + @Override + public List<String> 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 e0d4a2d7ae..7dfe640c8c 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 class DefaultGarbageCollectionStrategyTest { 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 0000000000..e505c3ea82 --- /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: + * + * <pre> + * 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 + * </pre> + * + * <p>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<CompactionResult> 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<CompactionResult> 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<CompactionResult> 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<CompactionResult> 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<CompactionResult> 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 a344df7419..e99560eec6 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 @@ import static org.apache.jackrabbit.oak.segment.file.FileStoreBuilder.fileStoreB * and pass once the fix that retains the {@code CompactionResult} across the two calls * is applied. */ -@Ignore public class OfflineCompactionGcLogTest { @Rule
