[
https://issues.apache.org/jira/browse/OAK-12119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18064496#comment-18064496
]
Rishabh Daim commented on OAK-12119:
------------------------------------
Why 1.22 works correctly
The fix is unnecessary on 1.22 because the whole requiresGCJournalEntry()
gating mechanism doesn't exist there. The 1.22 design is fundamentally
different in three ways:
Key difference 1 — DefaultCleanupStrategy always writes the journal (no null
check)
{code}1.22 (DefaultCleanupStrategy.java:65):
// No null check — always persists
context.getGCJournal().persist(
reclaimedSize, finalSize,
getGcGeneration(context),
context.getCompactionMonitor().getCompactedNodes(),
context.getCompactedRootId()
);
Trunk (DefaultCleanupStrategy.java):
GCJournal gcJournal = context.getGCJournal();
if (gcJournal != null) { // ← trunk added this null guard
gcJournal.persist(...);
}{code}
Key difference 2 — newCleanupStrategyContext on 1.22 never returns null for
getGCJournal()
{code}Trunk introduced this gate in
AbstractGarbageCollectionStrategy.newCleanupStrategyContext():
// Trunk only — blocks the journal when requiresGCJournalEntry() is false
return compactionResult.requiresGCJournalEntry() ? context.getGCJournal() :
null;
This method doesn't have this gate on 1.22 — it always returns
context.getGCJournal().{code}
Key difference 3 — CompactionResult.skipped() on 1.22 provides a valid
getCompactedRootId()
On 1.22, skipped() overrides getCompactedRootId() to return the real current
head RecordId. When store.cleanup() is called standalone,
AbstractGarbageCollectionStrategy.cleanup(Context) constructs a skipped(...)
using the current head revision — so DefaultCleanupStrategy always gets a
real root ID to write to the journal.
On trunk, CompactionResult added requiresGCJournalEntry() returning true only
for succeeded, and newCleanupStrategyContext was changed to return null for the
journal when this returns false. The skipped result
has requiresGCJournalEntry() = false, so getGCJournal() returns null in the
cleanup context, and DefaultCleanupStrategy's null check skips the write.
---
The full regression chain introduced in trunk
1.22:
cleanup(Context) → skipped(currentHead) → newCleanupStrategyContext()
→ getGCJournal() always returns real journal
→ DefaultCleanupStrategy: no null check → always persists ✅
trunk:
cleanup(Context) → skipped(currentHead) → newCleanupStrategyContext()
→ getGCJournal() returns compactionResult.requiresGCJournalEntry() ?
journal : null
→ skipped.requiresGCJournalEntry() = false → returns null
→ DefaultCleanupStrategy: if (gcJournal != null) → skips persist ❌
Three changes were made in trunk that each individually would be fine, but
together broke offline compaction:
1. requiresGCJournalEntry() added to CompactionResult (false for skipped)
2. Gate in newCleanupStrategyContext() that returns null journal when
!requiresGCJournalEntry()
3. Null check in DefaultCleanupStrategy
Our fix (lastCompactionResult field in GarbageCollector) correctly restores
the 1.22 behaviour for offline compaction by ensuring the standalone cleanup()
path uses the real succeeded result (with
requiresGCJournalEntry() = true) instead of the skipped placeholder.
> Offline Compaction does not persist compacted head into gc.log
> --------------------------------------------------------------
>
> Key: OAK-12119
> URL: https://issues.apache.org/jira/browse/OAK-12119
> Project: Jackrabbit Oak
> Issue Type: Bug
> Affects Versions: 1.92.0
> Reporter: Rishabh Daim
> Assignee: Rishabh Daim
> Priority: Critical
> Fix For: 2.0.0
>
>
> *Root Cause*
> The offline compaction in *Compact.run()* calls *compactFull()* and
> *cleanup()* as two separate calls, but the _CompactionResult_ from compaction
> is silently discarded between them.
>
> *Call chain:*
> Compact.run()
> ├─ store.compactFull()
> │ └─ garbageCollector.compactFull(strategy).isSuccess()
> │ ^^^^^^^^^^^
> │ CompactionResult.succeeded(...) is returned but thrown away!
> │
> └─ store.cleanup()
> └─ garbageCollector.cleanup(strategy)
> └─ strategy.cleanup(context) ← no-arg overload
> └─ AbstractGarbageCollectionStrategy.cleanup(Context):
> return cleanup(context, CompactionResult.skipped(...))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Always creates a SKIPPED
> result!
>
> Why *gc.log* is never written:
> In *AbstractGarbageCollectionStrategy.java:88–95:*
> // Standalone cleanup always creates a SKIPPED result
>
> {code:java}
> public List<String> cleanup(Context context) throws IOException {
> return cleanup(context, CompactionResult.skipped(...));
> }{code}
> *CompactionResult.skipped(...)* inherits *requiresGCJournalEntry()* from
> the base class, which returns false (line 216).
> Then in *newCleanupStrategyContext()* at line 295–297:
>
> {code:java}
> public GCJournal getGCJournal()
> { return compactionResult.requiresGCJournalEntry() ?
> context.getGCJournal() : null; // ↑ false for skipped → returns null!
> }{code}
> *DefaultCleanupStrategy* receives null for gcJournal, so it skips the
> gcJournal.persist(...) call entirely.
> Contrast with online (integrated) GC:
> The *AbstractGarbageCollectionStrategy.run()* method calls *cleanup(context,
> compactionResult)* with the actual *CompactionResult.succeeded(...)* — that's
> why gc.log works online
> but not offline.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
