feat(transaction): add the AddBase action and the conflict substrate - #8058
Draft
wjones127 wants to merge 1 commit into
Draft
feat(transaction): add the AddBase action and the conflict substrate#8058wjones127 wants to merge 1 commit into
wjones127 wants to merge 1 commit into
Conversation
Adds the first vertical slice of action-based transactions (Transaction V2): the in-memory vocabulary, an apply path, and footprint-based conflict resolution, with `AddBase` as the only action. `Operation::UserOperation` carries an ordered list of `UserAction` steps that expand to granular `Action` deltas and commit atomically. Minted ids travel as `Local` tokens and are allocated at apply, so two concurrent operations cannot collide on an id, and a retry simply re-mints from the newer manifest rather than needing rebase state. Conflicts are decided by intersecting `ManifestMask` footprints instead of matching operation pairs: each action and each legacy `Operation` declares the manifest regions it writes, and the intersection is computed once in `mask.rs`. Exact-key overlap (a taken base name or path) is incompatible; coarser overlap is retryable. `check_txn` answers a V2 `other` by footprint before any legacy dispatch, because several `check_*_txn` bodies mutate rebase state derived from the other operation's contents and would otherwise permit it silently while skipping that side effect. The existing `test_conflicts` table now doubles as an oracle for `Operation::writes()`: disjoint footprints must mean the table said `Compatible`, which catches an under-declared mask. `TryFrom<&Operation> for Vec<Action>` translates `UpdateBases` and backs a round-trip parity test asserting the legacy and action paths build identical manifests. Co-Authored-By: Claude Opus 5 (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.
Stacked on #7954, which drafts the wire format. This is the first working slice of action-based transactions (Transaction V2), implementing OSS-1542's
AddBaseand the machinery every later action reuses.Today a transaction is one
Operationdescribing one kind of change, and deciding whether two of them collide means consulting a pairwise matrix ofcheck_*_txnfunctions — one row and column per operation. That matrix is why compound commits and branch merges are hard: there is no way to say "this transaction does two things", and no way to combine two transactions that each minted an id.This PR adds a second, composable vocabulary alongside it.
Operation::UserOperationcarries an ordered list of steps that expand into granularActiondeltas and commit atomically. Newly allocated ids (base ids here, field and fragment ids later) travel asLocalplaceholder tokens and are assigned when the transaction is applied. That is what makes them safe under concurrency: each side allocates from whatever the counter holds at its own apply time, so two concurrent transactions can never pick the same id, and a retry just re-allocates against the newer manifest instead of needing rebase bookkeeping.Conflicts are decided by comparing footprints rather than operation pairs. Each action, and each existing
Operation, declares which regions of the manifest it writes (ManifestMask), and two transactions conflict exactly when those declarations intersect — computed once, in one place. Adding an action costs one declaration instead of a row and a column. Claiming a specific name (a base name or path, a config key) is an exclusive claim, so overlap there is incompatible; anything coarser is retryable, because a retry re-runs apply and apply re-validates.AddBaseis deliberately the smallest useful first action: it writes onlybase_paths, which is the one region where none of the existing rebase machinery needs to do extra work.Example
Two such transactions planned against the same version, adding different bases, both commit and receive distinct ids without any rebase step.
Notes for review
check_txnordering is the safety-critical part. A concurrent action-based transaction is answered by footprint before any legacy dispatch. Severalcheck_*_txnbodies do not merely decide — they also record state derived from the other transaction (fragments needing a deletion-file rewrite, conflicting frag-reuse indices, compacted SSTables) — and most end in a permissive arm, so one reaching them would be quietly allowed while the work it should have triggered never happened.REGIONS_WITH_REBASE_SIDE_EFFECTSgates the regions that still need those side effects; a later slice has to supply them before its region comes off the list. Every one of those legacy bodies also gained an explicit fail-safe arm, since Rust requires the match to be exhaustive and falling through to a permissive arm is exactly the failure being guarded against.The existing conflict table is now an oracle.
test_conflictsalready encodes the legacy verdict for every operation pair.Operation::writes()restates the same information as footprints, so the test now also asserts that disjoint footprints imply the table saidCompatible. Over-declaring passes (a spurious retry is harmless); an under-declared footprint, the direction that could let two colliding commits through, fails. As the plan predicted, one wrinkle is real: the legacy table is direction-sensitive (ReserveFragments ‖ Overwritediffers by direction) while intersection is symmetric, so the stricter direction wins. That is the acceptable failure mode, and the assertion still holds.Round-trip parity is the test that matters most.
TryFrom<&Operation> for Vec<Action>translatesUpdateBases, and a test drives the same change down both the legacy and the action path and compares the resulting manifests field by field. It is what would catch the action path quietly diverging.Deviations from the implementation plan
UserOperationon load, because that version had no support for it. This PR supports it, so the rejection now lives inAction's decode: an action this version does not recognize errors out rather than being skipped. That is the level that matters, since a leniently parsed action would vanish from conflict detection and let two colliding commits both succeed. The renamed test still covers it.UserOperation::build_manifesttakes the transaction'stag. The signature in the plan omitted it, but the plan also asks for the same scaffolding the legacy path applies, which includes the tag. Without the parameter every action-based commit would silently lose it.build_manifest, so the action path inherits that check rather than duplicating it.TxnContext::resolve_baseis not included. Nothing in this slice reads a base binding —AddDataFileis the first consumer — so it would be dead code.bind_baseand its distinctness check are here, as is a test-only accessor asserting the right id lands under the right token.Action::conflicts_withis not included. It would forward toself.writes().conflicts_with(&other.writes()); callers use the mask directly.name == nameunconditionally, so adding a second unnamed base to a dataset that already has one is wrongly rejected today. The legacy conflict check (check_add_bases_txn) already gets this right, so the action path agrees with it; the legacy apply path is untouched. There's a test for the corrected behavior.Rebase of #7954
Rebasing #7954 onto the refactor stack turned up real drift:
MergedGenerationwas renamed toCompactedSsTableupstream after that branch was cut, and #7954 recreatedtransaction.protofrom a pre-rename snapshot, so the protos no longer compiled. Fixed on #7954's branch (force-pushed, base retargeted torefactor/transaction-module-split), which also renames the draftUpdateMergedGenerationsaction toUpdateCompactedSsTablesso the action matches the type it carries. Two rustfmt fixes from the same rename went there too, so that branch is fmt-clean on its own.For Will
Two things from the plan I did not resolve on my own:
max_base_idinManifestortable.proto; the next id is derived from the current maximum key, and it only stays monotone because nothing in the tree ever removes a base. That is fine within a single lineage, which is all this PR does, but OSS-1529's relocation-by-watermark model assumes a never-reused counter, so it will need either a real counter field or a written-down never-remove invariant. Noted in a comment at the mint site. The irony is worth flagging: the mask design argues counters never conflict because they are monotone and never reused, which is exactly the property base ids lack on paper.Restoresilently drops concurrently-added bases.restore_old_manifestreplaces the manifest wholesale without mergingbase_pathsforward, yetcheck_add_bases_txnreturnsOkfor it — soUpdateBases ‖ Restoreloses the base today. Pre-existing; not fixed here. The action path givesRestorean everything-footprint, which makes the pairing retryable (re-adding on top of the restored state is correct).Not included
Every other action,
reads()(write-versus-write only for now; the plan records why, and the trap to avoid when read sets land), relocatingLocaltokens against a different target's counters for branch merge and squash, and Python/Java surfaces — the bindings just reject action-based operations.One test I could not write: the interception half of the frontier rule. Triggering it needs a concurrent action-based transaction whose footprint touches fragments, indices, or generations, and no action in this PR does. The gate's logic is covered at the
ManifestMasklevel, and the path it protects becomes reachable — and testable — with the first fragment-level action.Also worth stating plainly: nothing from #7954 upward should merge to
mainuntil the format vote (OSS-757) passes. The branch-level hold is the release gate, which is why there is no Cargo feature here.