You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by da...@apache.org on 2026/09/29 16:20:02 UTC
(jackrabbit-oak) branch trunk updated: OAK-12408 : Journal gc.log in CompactionResult.skipped() when current generation is already compacted (#3137)
This is an automated email from the ASF dual-hosted git repository.
rishabhdaim pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new 8d32f44fc7 OAK-12408 : Journal gc.log in CompactionResult.skipped() when current generation is already compacted (#3137)
8d32f44fc7 is described below
commit 8d32f44fc7035a3e607259b8f1b8393f33077f0d
Author: Shivam-Agg <47...@users.noreply.github.com>
AuthorDate: Tue Sep 29 21:49:55 2026 +0530
OAK-12408 : Journal gc.log in CompactionResult.skipped() when current generation is already compacted (#3137)
* Journal gc.log in CompactionResult.skipped() when current
generation is already compacted
Standalone cleanup() (cold standby, backup/restore) never
journaled gc.log, letting it go stale while segments were
still being reclaimed, causing SegmentNotFoundException after
standby promotion. Return currentGeneration.isCompacted()
instead of hard-coded false; safe since GCJournal.persist()
already no-ops on unchanged generation.
Co-authored-by: Copilot <22...@users.noreply.github.com>
* Add regression tests for standalone cleanup() gc.log journaling
Covers the cold-standby/backup scenario: a fresh FileStore instance
that never ran compactFull()/compactTail() itself must still journal
gc.log when the persisted head is already a compacted generation, and
must not journal when it isn't. Verified these fail against the
pre-fix CompactionResult.skipped() and pass with the fix.
Co-authored-by: Copilot <22...@users.noreply.github.com>
* Dedup gc.log entries by generation only, not isCompacted
GCJournalEntry.fromString() always deserializes isCompacted as false,
so comparing the full GCGeneration in GCJournal.persist() wrongly
treated an already-journaled, still-compacted generation as new on
every restart, writing duplicate entries.
* Add unit test for skipped()+isCompacted; strengthen gen assertion
- New unit test: skipped() with isCompacted=true persists to journal.
- StandaloneCleanupGcLogTest now checks exact generation/fullGeneration
instead of a weak >= 1 bound.
---------
Co-authored-by: shivamagg <sh...@adobe.com>
Co-authored-by: Copilot <22...@users.noreply.github.com>
---
.../oak/segment/file/CompactionResult.java | 9 ++
.../jackrabbit/oak/segment/file/GCJournal.java | 10 +-
.../file/DefaultGarbageCollectionStrategyTest.java | 12 ++
.../jackrabbit/oak/segment/file/GcJournalTest.java | 20 +++
.../segment/file/StandaloneCleanupGcLogTest.java | 136 +++++++++++++++++++++
5 files changed, 186 insertions(+), 1 deletion(-)
diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/CompactionResult.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/CompactionResult.java
index 99996990e5..0fd375d596 100644
--- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/CompactionResult.java
+++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/CompactionResult.java
@@ -166,6 +166,15 @@ abstract class CompactionResult {
RecordId getCompactedRootId() {
return compactedRootId;
}
+
+ // A standalone cleanup() call (no compaction ran in this JVM) still needs to journal
+ // the GC state if the current head is already a compacted generation that was never
+ // recorded in this store's own gc.log - e.g. a cold standby that received a compacted
+ // head via sync and is now reclaiming the superseded generation locally.
+ @Override
+ boolean requiresGCJournalEntry() {
+ return currentGeneration.isCompacted();
+ }
};
}
diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java
index 6e3d398510..92a140d552 100644
--- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java
+++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java
@@ -68,7 +68,15 @@ public class GCJournal {
@NotNull GCGeneration gcGeneration, long nodes, @NotNull String root
) {
GCJournalEntry current = read();
- if (current.getGcGeneration().equals(gcGeneration)) {
+ GCGeneration currentGeneration = current.getGcGeneration();
+ // Compare generation and fullGeneration only, not isCompacted: the on-disk journal
+ // format never persists isCompacted (GCJournalEntry.fromString always deserializes it
+ // as false), so comparing the full GCGeneration would wrongly treat every persist() of
+ // an already-journaled, still-compacted generation as new after a restart (e.g. a
+ // standalone cleanup() on a store re-opened with an already-compacted head), causing a
+ // duplicate entry to be written on every such restart.
+ if (currentGeneration.getGeneration() == gcGeneration.getGeneration()
+ && currentGeneration.getFullGeneration() == gcGeneration.getFullGeneration()) {
// failed compaction, only update the journal if the generation
// increases
return;
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 20b92dba70..d67fc64371 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
@@ -108,6 +108,18 @@ public class DefaultGarbageCollectionStrategyTest {
verifyGCJournalPersistence(Mockito.never());
}
+ @Test
+ public void skippedCompactionWithCompactedGenerationPersistsToJournal() throws Exception {
+ CompactionResult result = CompactionResult.skipped(
+ SegmentGCOptions.GCType.FULL,
+ GCGeneration.newGCGeneration(1, 1, true),
+ SegmentGCOptions.defaultGCOptions(),
+ RecordId.NULL,
+ 0);
+ runCleanup(result);
+ verifyGCJournalPersistence(Mockito.times(1));
+ }
+
@Test
public void nonApplicableCompactionDoesNotPersistToJournal() throws Exception {
runCleanup(CompactionResult.notApplicable(0));
diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java
index 7e17c0df1c..0a87ba14cc 100644
--- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java
+++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java
@@ -105,6 +105,26 @@ public class GcJournalTest {
assertEquals(newGCGeneration(1, 2, false), in.read().getGcGeneration());
}
+ /**
+ * A freshly opened {@code GCJournal} (simulating a restart / a different JVM) reads its
+ * latest entry from disk, where the compacted flag is always deserialized as {@code false}
+ * (see {@link #testGCGenerationCompactedFlagCleared}). Persisting again for the same
+ * generation - but with {@code isCompacted=true}, as a live store head always reports -
+ * must still be treated as a no-op; otherwise every restart of a store whose head sits on
+ * an already-journaled compacted generation would write a duplicate gc.log entry.
+ */
+ @Test
+ public void testPersistIsNoopAcrossRestartForSameGeneration() throws Exception {
+ GCJournal out = new GCJournal(getPersistence().getGCJournalFile());
+ out.persist(1, 100, newGCGeneration(1, 1, true), 50, "foo");
+
+ GCJournal afterRestart = new GCJournal(getPersistence().getGCJournalFile());
+ afterRestart.persist(0, 100, newGCGeneration(1, 1, true), 0, "foo");
+
+ assertEquals("gc.log must not gain a duplicate entry for the same generation "
+ + "after a restart", 1, afterRestart.readAll().size());
+ }
+
@Test
public void testReadOak16GCLog() throws Exception {
createOak16GCLog();
diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/StandaloneCleanupGcLogTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/StandaloneCleanupGcLogTest.java
new file mode 100644
index 0000000000..6df9f1adf4
--- /dev/null
+++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/StandaloneCleanupGcLogTest.java
@@ -0,0 +1,136 @@
+/*
+ * 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 static org.apache.jackrabbit.oak.segment.compaction.SegmentGCOptions.defaultGCOptions;
+import static org.apache.jackrabbit.oak.segment.file.FileStoreBuilder.fileStoreBuilder;
+
+import java.io.File;
+
+import org.apache.jackrabbit.oak.segment.SegmentNodeBuilder;
+import org.apache.jackrabbit.oak.segment.SegmentNodeState;
+import org.apache.jackrabbit.oak.segment.file.GCJournal.GCJournalEntry;
+import org.apache.jackrabbit.oak.segment.file.tar.TarPersistence;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+/**
+ * Tests that a standalone {@code cleanup()} call - i.e. one with no preceding
+ * {@code compactFull()}/{@code compactTail()} on the same {@code FileStore} instance,
+ * exactly the sequence used by {@code StandbyClientSync} after a sync cycle, and by
+ * {@code FileStoreBackupImpl}/{@code FileStoreBackupRestoreMBean} - still journals gc.log
+ * whenever the persisted head is already a compacted generation.
+ *
+ * <p>Before the fix, {@code CompactionResult.skipped(...)}'s {@code requiresGCJournalEntry()}
+ * was hard-coded to {@code false}, so gc.log was never updated for this call path, even
+ * though the reclaimer actively removes segments belonging to older generations. This left
+ * gc.log pointing at a stale root, which can later cause a {@code SegmentNotFoundException}.
+ */
+public class StandaloneCleanupGcLogTest {
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder(new File("target"));
+
+ /**
+ * Simulates a cold standby: one {@code FileStore} instance compacts and persists a new,
+ * compacted head (like a primary would), then a second, freshly opened {@code FileStore}
+ * instance - which never compacted anything itself - calls the standalone
+ * {@code cleanup()} (like {@code StandbyClientSync} does after a sync cycle). gc.log must
+ * be updated to reflect the compacted head that this second instance found on disk.
+ */
+ @Test
+ public void testStandaloneCleanupPersistsGcLogForCompactedHead() throws Exception {
+ File storeDir = folder.getRoot();
+
+ // Step 1: populate the store and compact it, persisting a compacted head to disk.
+ // No cleanup() is called here, mirroring a primary that compacted while this
+ // instance never observed the compaction locally.
+ try (FileStore store = fileStoreBuilder(storeDir)
+ .withGCOptions(defaultGCOptions().setOffline())
+ .build()) {
+ SegmentNodeState base = store.getHead();
+ SegmentNodeBuilder builder = base.builder();
+ builder.setProperty("key", "value");
+ store.getRevisions().setHead(base.getRecordId(), builder.getNodeState().getRecordId());
+ store.flush();
+
+ Assert.assertTrue("compactFull() should succeed", store.compactFull());
+ }
+
+ // Step 2: gc.log must still be empty - nothing has called cleanup() yet.
+ GCJournal gcJournalBefore = new GCJournal(new TarPersistence(storeDir).getGCJournalFile());
+ Assert.assertEquals("gc.log should be empty before any cleanup() call",
+ GCJournalEntry.EMPTY, gcJournalBefore.read());
+
+ // Step 3: open a fresh FileStore instance - it never called compactFull()/compactTail()
+ // itself - and call the standalone, no-arg cleanup(), exactly as StandbyClientSync does.
+ try (FileStore store = fileStoreBuilder(storeDir)
+ .withGCOptions(defaultGCOptions().setOffline())
+ .build()) {
+ store.cleanup();
+ }
+
+ // Step 4: gc.log must now record the compacted head found on disk.
+ GCJournal gcJournalAfter = new GCJournal(new TarPersistence(storeDir).getGCJournalFile());
+ GCJournalEntry entry = gcJournalAfter.read();
+
+ Assert.assertNotEquals(
+ "gc.log must have a non-empty entry after a standalone cleanup() finds an "
+ + "already-compacted head",
+ GCJournalEntry.EMPTY, entry);
+ // Note: GCJournalEntry serialization always clears the compacted flag on the persisted
+ // generation (see GcJournalTest#testGCGenerationCompactedFlagCleared) - the generation
+ // number itself is what proves this entry reflects the compacted head. A single
+ // compactFull() on a fresh store always produces generation=1, fullGeneration=1.
+ Assert.assertEquals("gc.log entry must record generation 1",
+ 1, entry.getGcGeneration().getGeneration());
+ Assert.assertEquals("gc.log entry must record full generation 1",
+ 1, entry.getGcGeneration().getFullGeneration());
+ }
+
+ /**
+ * A standalone {@code cleanup()} on a store whose head was never compacted must not
+ * write a gc.log entry - this preserves the original, correct behavior for fresh
+ * repositories that have never gone through compaction.
+ */
+ @Test
+ public void testStandaloneCleanupDoesNotPersistGcLogForUncompactedHead() throws Exception {
+ File storeDir = folder.getRoot();
+
+ try (FileStore store = fileStoreBuilder(storeDir)
+ .withGCOptions(defaultGCOptions().setOffline())
+ .build()) {
+ SegmentNodeState base = store.getHead();
+ SegmentNodeBuilder builder = base.builder();
+ builder.setProperty("key", "value");
+ store.getRevisions().setHead(base.getRecordId(), builder.getNodeState().getRecordId());
+ store.flush();
+
+ store.cleanup();
+ }
+
+ GCJournal gcJournal = new GCJournal(new TarPersistence(storeDir).getGCJournalFile());
+ Assert.assertEquals(
+ "gc.log must stay empty when cleanup() runs on a never-compacted head",
+ GCJournalEntry.EMPTY, gcJournal.read());
+ }
+
+}