HDDS-15822. Fix flaky TestStorageVolumeChecker#testNumScansSkipped#10720
Merged
Conversation
Use a synchronous direct-executor ThrottledAsyncChecker and diskCheckTimeout=0 so completedChecks is fully populated before checkAllVolumes returns, removing the race with timer.advance() that intermittently inflated numScansSkipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a flake in TestStorageVolumeChecker#testNumScansSkipped by removing a timing race between asynchronous volume-check completion callbacks and the test’s FakeTimer.advance() calls. It does this by making the delegate checker effectively synchronous for this specific test case, while keeping the throttling behavior being asserted.
Changes:
- Configure
diskCheckTimeout=0for the test’sStorageVolumeCheckerinstance. - Replace the default async delegate with a
ThrottledAsyncCheckerbacked by a direct executor to ensurecompletedChecksis updated beforecheckAllVolumesreturns.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jojochuang
approved these changes
Jul 11, 2026
Contributor
Author
|
Thanks @jojochuang for reviewing this. |
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.
Generated-by: Claude Code (Opus 4.8)
What changes were proposed in this pull request?
TestStorageVolumeChecker#testNumScansSkippedfails intermittently withexpected: <1> but was: <2>(seen in upstream CI on 2026-07-09).The test uses the real
ThrottledAsyncChecker, which runs volume checks on a background executor and wraps them withFutures.withTimeout. The callback that records each check'scompletedAtruns on those executor threads. On a busy JVM this races with the test'stimer.advance(), so a check'scompletedAtis occasionally recorded against a later timer value. On the thirdcheckAllVolumescall the affected volume then looks like it is still withinminMsBetweenChecksand is skipped, which inflatesnumScansSkippedfrom 1 to 2.This PR makes the check synchronous for this test by installing a
ThrottledAsyncCheckerbacked by a direct executor service, and by settingdiskCheckTimeoutto zero so noFutures.withTimeoutwrapper is created.completedChecksis then fully populated beforecheckAllVolumesreturns, which removes the race. The throttling gap (getDiskCheckMinGap) is unchanged, so the test still verifies skip counting, and the asynchronous per volume timeout path stays covered by the dedicated test added in HDDS-14871. The change is limited to the test.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15822
How was this patch tested?
Ran
testNumScansSkipped100 times locally with 0 failures. Ran the fullTestStorageVolumeCheckerclass (20 tests) multiple times with no failures. Checkstyle clean.