Core: Add branch support for RewriteManifests operation#16645
Open
wombatu-kun wants to merge 1 commit into
Open
Core: Add branch support for RewriteManifests operation#16645wombatu-kun wants to merge 1 commit into
wombatu-kun wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds branch support to
RewriteManifestsviatoBranch(String)so manifest compaction/clustering can target a named branch. Closes #15981.RewriteManifestswas the onlySnapshotUpdatethat could not commit to a branch:BaseRewriteManifestsinherited the defaultSnapshotUpdate.toBranch, which throwsUnsupportedOperationException. There was also a second, latent bug: even iftoBranchwere unblocked,apply(base, snapshot)read manifests frombase.currentSnapshot()(always themaintip) rather than thesnapshotargument thatSnapshotProducerpasses in (the tip of the target branch), so a branch commit would have rewritten the wrong manifests.Changes
toBranchto delegate to the inheritedSnapshotProducer.targetBranch, which already rejectsnulland tag refs withIllegalArgumentException.snapshotparameter (snapshot != null ? snapshot.allManifests(io) : Collections.emptyList()), mirroringFastAppend.applyandMergingSnapshotProducer.apply.Rewriting an existing branch now compacts that branch's manifests without affecting
main. Targeting a branch that does not exist yet forks it frommain(parent = the currentmainsnapshot), consistent with otherSnapshotUpdateoperations.Tests
New tests in
TestRewriteManifests, exercised across format versions v1-v4:testRewriteManifestsOnBranch: the branch diverges frommain(an extra file appended only on the branch) before the rewrite, so it verifies the rewrite operates on the branch tip and leavesmainuntouched. This is the case that actually exercises theapply()fix.testRewriteManifestsCreatesBranchIfNeeded: rewriting to a non-existent branch forks it frommain.testRewriteManifestsToBranchRejectsNullBranchandtestRewriteManifestsToBranchRejectsTag: 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
mainbefore branching, somainand the branch were identical at rewrite time and the test did not exercise theapply()fix.