perf(test): poll is_compacting instead of a fixed sleep in replication test#3561
Merged
git-hulk merged 2 commits intoJul 20, 2026
Merged
Conversation
…n test TestReplicationShareCheckpoint issued `COMPACT` and then blindly `time.Sleep(time.Second)` before starting the replicas. This is both slower than necessary and potentially flaky: on a loaded CI runner one second may not be enough for the manual compaction to finish, so the test could proceed while compaction is still running. Replace the fixed sleep with the same `is_compacting` poll already used elsewhere in this file (TestReplicationWithHole, lines ~349-351), waiting up to 10s at a 100ms interval. This returns as soon as compaction completes and removes the blind wait. Part of apache#3524, addresses apache#3527. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the stability and runtime of the Go replication integration test suite by removing a fixed time.Sleep(1 * time.Second) after triggering manual compaction and instead waiting deterministically for compaction to finish via the is_compacting INFO field (matching an existing pattern already used in the same file).
Changes:
- Replace a fixed sleep in
TestReplicationShareCheckpointwithrequire.Eventuallypolling onis_compacting == "no". - Reduce potential test flakiness on slow/loaded CI runners while also avoiding unnecessary waiting when compaction completes quickly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
Hi @ishita-0301. If you can't run the tests locally, you can use the GitHub Actions workflow in your repository: https://github.com/ishita-0301/kvrocks/actions to make sure all tests are passing. |
jihuayu
approved these changes
Jul 20, 2026
git-hulk
approved these changes
Jul 20, 2026
git-hulk
enabled auto-merge (squash)
July 20, 2026 12:12
|
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 this does
TestReplicationShareCheckpointrunsCOMPACTand then blindlytime.Sleep(time.Second)before starting the replicas. That is slowerthan necessary and potentially flaky — on a loaded CI runner one second
may not be enough for the manual compaction to finish, so the test can
proceed while compaction is still running.
This replaces the fixed sleep with the same
is_compactingpoll alreadyused elsewhere in the same file (
TestReplicationWithHole), returning assoon as compaction completes:
Related
Part of #3524, addresses #3527.
Note on testing
I was not able to run the full Go integration suite locally for this
change; it mirrors an existing, already-tested pattern in the same file
rather than introducing new behavior. Happy to adjust if maintainers
prefer a different approach.