Skip to content

Core: Add branch support for RewriteManifests operation#16645

Open
wombatu-kun wants to merge 1 commit into
apache:mainfrom
wombatu-kun:issue/15981-rewrite-manifests-branch
Open

Core: Add branch support for RewriteManifests operation#16645
wombatu-kun wants to merge 1 commit into
apache:mainfrom
wombatu-kun:issue/15981-rewrite-manifests-branch

Conversation

@wombatu-kun
Copy link
Copy Markdown
Contributor

What

Adds branch support to RewriteManifests via toBranch(String) so manifest compaction/clustering can target a named branch. Closes #15981.

RewriteManifests was the only SnapshotUpdate that could not commit to a branch: BaseRewriteManifests inherited the default SnapshotUpdate.toBranch, which throws UnsupportedOperationException. There was also a second, latent bug: even if toBranch were unblocked, apply(base, snapshot) read manifests from base.currentSnapshot() (always the main tip) rather than the snapshot argument that SnapshotProducer passes in (the tip of the target branch), so a branch commit would have rewritten the wrong manifests.

Changes

  • Override toBranch to delegate to the inherited SnapshotProducer.targetBranch, which already rejects null and tag refs with IllegalArgumentException.
  • Read the current manifests from the snapshot parameter (snapshot != null ? snapshot.allManifests(io) : Collections.emptyList()), mirroring FastAppend.apply and MergingSnapshotProducer.apply.

Rewriting an existing branch now compacts that branch's manifests without affecting main. Targeting a branch that does not exist yet forks it from main (parent = the current main snapshot), consistent with other SnapshotUpdate operations.

Tests

New tests in TestRewriteManifests, exercised across format versions v1-v4:

  • testRewriteManifestsOnBranch: the branch diverges from main (an extra file appended only on the branch) before the rewrite, so it verifies the rewrite operates on the branch tip and leaves main untouched. This is the case that actually exercises the apply() fix.
  • testRewriteManifestsCreatesBranchIfNeeded: rewriting to a non-existent branch forks it from main.
  • testRewriteManifestsToBranchRejectsNullBranch and testRewriteManifestsToBranchRejectsTag: validation.

Notes

This revives the approach from the stale PR #15982 by @jlkvanloon (added as a commit co-author), with broader test coverage. In particular, the original happy-path test appended both files to main before branching, so main and the branch were identical at rewrite time and the test did not exercise the apply() fix.

RewriteManifests was the only SnapshotUpdate that could not target a named branch: BaseRewriteManifests inherited the default SnapshotUpdate.toBranch, which throws UnsupportedOperationException. Even if that were unblocked, apply() read manifests from base.currentSnapshot() (always the main tip) instead of the snapshot parameter SnapshotProducer passes in (the tip of the target branch), so a branch commit would have rewritten the wrong manifests.

This overrides toBranch to delegate to the inherited targetBranch validation (which rejects null and tag refs with IllegalArgumentException) and reads the current manifests from the snapshot parameter, mirroring FastAppend and MergingSnapshotProducer. Rewriting an existing branch now compacts that branch's manifests without affecting main; targeting a branch that does not exist yet forks it from main, consistent with other SnapshotUpdate operations. This enables per-branch manifest clustering and Write-Audit-Publish workflows.

The implementation revives the approach from the stale PR apache#15982 by Joaquin van Loon, with additional tests covering branch/main divergence, branch creation, and null/tag validation.

Closes apache#15981

Co-Authored-By: Joaquin van Loon <joaquin.vanloon@imc.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the core label Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add branch support for RewriteManifests operation

1 participant