-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[ML] Fix flaky ForecastIT#testOverflowToDisk #137452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Fix flaky ForecastIT#testOverflowToDisk #137452
Conversation
…Added logic to wait for forecast documents to exist in non-terminal states (SCHEDULED, STARTED) before checking for FINISHED status. Included index refresh to ensure visibility of recently indexed forecast stats documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a flaky test (ForecastIT#testOverflowToDisk) by addressing race conditions in forecast status verification. The fix ensures the forecast document is visible and properly tracked through its state transitions before asserting completion.
Key Changes:
- Modified
waitForecastToFinishto perform a two-stage wait: first for the forecast to reach any non-terminal state (SCHEDULED/STARTED/FINISHED), then specifically for FINISHED status - Added index refresh in
waitForecastStatusto ensure recently indexed forecast documents are visible before status checks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int timeoutSeconds = inFipsJvm() ? 300 : 90; | ||
| // First wait for the forecast document to exist and be in a non-terminal state | ||
| // This handles the race condition where the document may be SCHEDULED or STARTED initially | ||
| waitForecastStatus( | ||
| timeoutSeconds, | ||
| jobId, | ||
| forecastId, | ||
| ForecastRequestStats.ForecastRequestStatus.SCHEDULED, | ||
| ForecastRequestStats.ForecastRequestStatus.STARTED, | ||
| ForecastRequestStats.ForecastRequestStatus.FINISHED | ||
| ); |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'flacky' to 'flaky' in the PR title.
|
Pinging @elastic/ml-core (Team:ML) |
davidkyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What's going on:
Race condition:
Hence, this PR mitigates the possible race condition by doing two things:
Fixes #117740