Decouple CoordinatorCommitHandler from TransactionContext and WriteSetEncoder#3658
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Consensus Commit transaction commit handler to decouple CoordinatorCommitHandler from WriteSetEncoder. The orchestrators (CommitHandler and CommitHandlerWithGroupCommit) now handle write set encoding before delegating to the coordinator handlers, and the group commit emitter merges pre-encoded write sets using a new CoordinatorGroupCommitValue carrier. Feedback on the changes highlights a potential issue where commitStateWithoutWriteSet and abortStateWithoutWriteSet are not overridden in CommitHandlerWithGroupCommit, which could bypass the group committer and lead to slot leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Refactors Consensus Commit coordinator-state writing to be driven by transaction ID plus a pre-encoded WriteSet, decoupling CoordinatorCommitHandler (and the group-commit emitter) from snapshot-bearing TransactionContext and from WriteSetEncoder. This supports upcoming TwoPhaseCommit flows that need to write coordinator state by ID without holding a transaction snapshot.
Changes:
CoordinatorCommitHandlerAPIs now accept(txId, WriteSet?, committedAt)for commit and(txId, WriteSet?)for abort; conflict handling is ID-based.- Orchestrator (
CommitHandler/CommitHandlerWithGroupCommit) now ownsWriteSetEncoderand supplies encoded write sets (and non-groupcommittedAt) when delegating. - Group commit now buffers a
(fullId, WriteSet?)carrier (CoordinatorGroupCommitValue), and the emitter merges/stamps per-child entry groups at emit time; tests updated accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandler.java | Moves write-set encoding (and non-group committedAt) responsibility into the orchestrator before delegating to coordinator handler. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java | Routes coordinator commit-state writes through group commit using encoded write sets; makes slot cancellation ID-based. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorCommitHandler.java | Converts coordinator-side state writes/conflict handling to operate on txId + pre-encoded WriteSet. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorCommitHandlerWithGroupCommit.java | Updates group-commit path to buffer (fullId, WriteSet) values and merge/stamp at emit time; renames APIs to avoid accidental binding. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitter.java | Updates the group committer value type to CoordinatorGroupCommitValue. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitValue.java | Introduces a carrier for group-commit buffering: full transaction ID + pre-encoded WriteSet. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/WriteSetEncoder.java | Removes now-unused multi-group encoding logic and its key-manipulator dependency. |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java | Updates orchestration tests to reflect ID-based coordinator handler APIs and verify orchestrator encoding delegation. |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java | Adjusts group-commit orchestration assertions to target groupCommitState and ID-based cancellation. |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorCommitHandlerTest.java | Reworks direct coordinator-handler tests to pass IDs and pre-encoded write sets. |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorCommitHandlerWithGroupCommitTest.java | Reworks group-commit handler/emitter tests to use pre-encoded write sets and value carriers. |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitterTest.java | Updates group-committer tests to buffer CoordinatorGroupCommitValue instead of TransactionContext. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5cf2d71 to
3a785d0
Compare
3a785d0 to
2133628
Compare
270077c to
5f13f8b
Compare
ad1a28e to
130dda4
Compare
5f13f8b to
3d12a9b
Compare
a25871a to
ddf5b2a
Compare
3d12a9b to
756e519
Compare
…pendent Decouple the Coordinator-side commit handler from TransactionContext and WriteSetEncoder. CoordinatorCommitHandler now operates on a transaction id plus a pre-encoded WriteSet, so the orchestrator (CommitHandler) owns the WriteSetEncoder and supplies the encoded write set and committedAt. For group commit, the Emittable value becomes a (fullId, WriteSet) carrier (CoordinatorGroupCommitValue) so the Emitter is encoder-free: it derives each child id from the carried full id and stamps the pre-encoded write set. The WriteSetEncoder therefore lives only in the orchestrator layer, and WriteSetEncoder#encodeMultiGroupWriteSet and its key-manipulator dependency are dropped. The group-commit entry point is named groupCommitState (distinct from the base commitState, whose signature differs) so a caller cannot silently bind to the base direct-putState overload, and slot cancellation is id-based (cancelGroupCommit). Tests are reworked to the id-based signatures, with commitState delegation routed through protected hooks so the group-commit subclass asserts the 2-arg groupCommitState (committedAt determined at emit time) while the base asserts the 3-arg orchestrator-provided variant.
ddf5b2a to
fe31825
Compare
Description
CoordinatorCommitHandlerwrites only the Coordinator state table, yet it took a fullTransactionContextand encoded the write set itself viaWriteSetEncoder, tying it to the snapshot-bearing execution context. This PR decouples the handler so it operates on a transaction ID plus a pre-encodedWriteSet, moving write-set encoding ownership up to the orchestrator (CommitHandler). This sharpens the layering — the Coordinator-side state logic now depends only on what it actually writes — and lets those state writes be driven by ID.Related issues and/or PRs
Changes made
CoordinatorCommitHandler:commitState/abortState/handleCommitConflictnow take a transaction ID and a pre-encodedWriteSet(commitStatealso takes thecommittedAtto stamp). The handler no longer referencesTransactionContextorWriteSetEncoder.CommitHandler(orchestrator): now owns theWriteSetEncoder, encodes the write set, and suppliescommittedAtbefore delegating.commitState/abortStateare encoding delegators (not thin pass-throughs), which is documented on the class.Emittablevalue is now a(fullId, WriteSet)carrier (newCoordinatorGroupCommitValue), so theEmitteris encoder-free — it derives each child ID from the carried full ID and stamps the pre-encoded write set. The group-commit entry point is namedgroupCommitState(distinct from the basecommitState, whose signature differs) to prevent a caller from silently binding to the base direct-putStateoverload, and slot cancellation is ID-based (cancelGroupCommit).WriteSetEncoder#encodeMultiGroupWriteSetand itsCoordinatorGroupCommitKeyManipulatordependency (now unused).commitStatedelegation is routed through protected hooks so the group-commit subclass asserts the 2-arggroupCommitStatewhile the base asserts the 3-arg orchestrator-provided variant.Checklist
Additional notes (optional)
N/A
Release notes
N/A