Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes custom transaction procedure replay in cluster replication by skipping cluster slot ownership verification during AOF replay (replica-side), relying on the invariant that the primary only writes verifiable operations to the AOF.
Changes:
- Add an
isReplayingpath for stored procedure execution during AOF replay and propagate it through the replay coordinator/session/transaction manager. - Skip slot ownership verification (
VerifyKeyOwnership) when a transaction procedure is running under replay. - Update cluster replication tests to cover stored proc replication with both object-store and main-store transaction procs, and adjust slot-verification error handling/logging.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test.cluster/ReplicationTests/ClusterReplicationBaseTests.cs | Expands stored-proc replication test matrix and registers either an object-store or main-store transaction proc; refactors replication attach logic. |
| libs/server/Transaction/TxnKeyManager.cs | Skips cluster slot verification when IsReplaying is true. |
| libs/server/Transaction/TransactionManager.cs | Introduces replay flag plumbing (isReplaying/IsReplaying) for transaction procs and uses it to adjust finalize behavior. |
| libs/server/Custom/CustomRespCommands.cs | Updates stored-proc execution entry point to pass replay intent to the transaction manager. |
| libs/server/AOF/ReplayCoordinator/AofReplayCoordinator.cs | Marks stored proc execution during AOF replay with isReplaying: true. |
| libs/cluster/Session/SlotVerification/RespClusterIterativeSlotVerify.cs | Avoids emitting slot verification responses when OK and adds logging on verification failure. |
| libs/cluster/Session/SlotVerification/ClusterSlotVerificationResult.cs | Adds ToString() to improve slot verification result logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/cluster/Session/SlotVerification/RespClusterIterativeSlotVerify.cs
Outdated
Show resolved
Hide resolved
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.
This PR resolves an issue with slot verification during custom transaction procedure replay.
The replica does not need to re-verify slots because verification is handled by the primary.
Any write that fails verification on the primary is never written to the AOF and therefore cannot reach the replica.
Note: The same invariant applies during migration, where the primary transfers slots and their associated keys to another primary.