Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two flaky test issues: (1) race conditions in tests caused by calling cancel() before Close(), and (2) timeout issues during the cutover phase due to an excessively high trivial threshold for unflushed changes.
- Reduced
binlogTrivialThresholdfrom 10,000 to 1,000 changes to prevent flush timeouts during the cutover lock phase - Reordered
Close()to execute beforecancel()in all test functions to avoid context cancellation race conditions - Added comprehensive documentation explaining the tradeoffs of the lower threshold value
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/repl/client.go | Reduced binlogTrivialThreshold constant from 10,000 to 1,000 and added detailed documentation explaining the tradeoff between cutover responsiveness and lock duration |
| pkg/migration/runner_resume_test.go | Fixed test flakiness by calling Close() before cancel() in four test functions (TestCheckpointResumeDuringChecksum, TestResumeFromCheckpointE2E, TestResumeFromCheckpointCompositeVarcharPK, TestResumeFromCheckpointStrict, TestResumeFromCheckpointE2EWithManualSentinel) to prevent race conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
58cc67d to
28240d6
Compare
28240d6 to
2027a09
Compare
5be46a2 to
1926f54
Compare
1926f54 to
c97a0df
Compare
c1ee5c0 to
e568300
Compare
7f58d63 to
fcd0a0f
Compare
f27cc91 to
a75e388
Compare
nonbb
approved these changes
Jan 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
A Pull Request should be associated with an Issue.
The first issue:
The cause is the
cancel()running too early and leaving a context cancelled error. If we run it afterClose()is ensures a cleaner finish. Fixes #552The second issue:
Spirit incorrectly handles RotateEvent, leading to events potentially being skipped in resuming from a checkpoint. Fixes #548
This can be a serious issue, but the steps required to reproduce it are quite specific. See #548 for discussion on it. I will merge this, deploy it to our staging environment and then likely cherry pick a release-branch version of it.