Skip to content

OAK-12119 : offline compaction does not persist compacted head into gc.log#2779

Merged
rishabhdaim merged 4 commits intotrunkfrom
OAK-12119
Mar 5, 2026
Merged

OAK-12119 : offline compaction does not persist compacted head into gc.log#2779
rishabhdaim merged 4 commits intotrunkfrom
OAK-12119

Conversation

@rishabhdaim
Copy link
Contributor

Summary

  • Offline compaction (compactFull() + cleanup() as separate calls, as used by oak-run compact) failed to write a gc.log entry because the CompactionResult from compactFull() was discarded before cleanup() ran
  • Fix stores the CompactionResult in GarbageCollector after compactFull()/compactTail() and passes it directly to the 2-arg cleanup(Context, CompactionResult) overload
  • Added cleanup(Context, CompactionResult) to the GarbageCollectionStrategy interface so GarbageCollector can call it through the interface type

Changes

  • GarbageCollectionStrategy.java — add cleanup(Context, CompactionResult) to the interface
  • GarbageCollector.java — store lastCompactionResult after compaction; pass it to cleanup, falling back to a skipped result when absent
  • SynchronizedGarbageCollectionStrategy.java — implement the new 2-arg cleanup overload
  • DefaultGarbageCollectionStrategyTest.java — update test setup for the fix
  • GarbageCollectorOfflineCompactionTest.java — new integration tests at GarbageCollector level using ArgumentCaptor to verify gc.log persistence
  • OfflineCompactionGcLogTest.java — end-to-end regression tests against a real FileStore

Test Plan

  • GarbageCollectorOfflineCompactionTest — verifies correct cleanup overload is called with the right CompactionResult
  • OfflineCompactionGcLogTest — verifies gc.log is written after offline compaction (regression tests, fail on unfixed code)
  • DefaultGarbageCollectionStrategyTest — existing tests still pass

Links

@rishabhdaim rishabhdaim requested a review from dulceanu March 4, 2026 12:06
@rishabhdaim rishabhdaim self-assigned this Mar 4, 2026
@rishabhdaim rishabhdaim requested a review from reschke March 4, 2026 12:06
@rishabhdaim
Copy link
Contributor Author

@lweitzendorf could you please review the changes ?

@rishabhdaim rishabhdaim requested a review from smiroslav March 4, 2026 21:16
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lacking detailed knowledge of publish persistence, I can only say: it has tests that prove the bug and the fix. Good.

FWIW, at least one of the Sonar warnings might be of interest.

@rishabhdaim
Copy link
Contributor Author

Lacking detailed knowledge of publish persistence, I can only say: it has tests that prove the bug and the fix. Good.

FWIW, at least one of the Sonar warnings might be of interest.

Addressed in 3635689

Copy link
Contributor

@lweitzendorf lweitzendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the fix!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2026

@rishabhdaim rishabhdaim merged commit aa9c569 into trunk Mar 5, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants