Remove sleep and change expectations for test_submit_exceed_max_rate#7313
Merged
roryharr merged 2 commits intoanza-xyz:masterfrom Aug 5, 2025
Merged
Remove sleep and change expectations for test_submit_exceed_max_rate#7313roryharr merged 2 commits intoanza-xyz:masterfrom
roryharr merged 2 commits intoanza-xyz:masterfrom
Conversation
steviez
requested changes
Aug 4, 2025
steviez
left a comment
There was a problem hiding this comment.
I'm looking at this one right now, give me a bit to finish reviewing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7313 +/- ##
=======================================
Coverage 82.8% 82.8%
=======================================
Files 801 801
Lines 363318 363315 -3
=======================================
+ Hits 300877 300886 +9
+ Misses 62441 62429 -12 🚀 New features to boost your workflow:
|
0182a9a to
a12ad77
Compare
steviez
approved these changes
Aug 5, 2025
steviez
left a comment
There was a problem hiding this comment.
Rory and I jumped on a call to discuss this one real quick. Reducing the wait addressed the issue, but still left this test racy.
Instead, we removed the sleep and took advantage of the Barrier that MetricsAgent::flush() creates. The result is no more raciness + no more sleep.
|
@roryharr - Since we changed direction with the PR, can you change the title ? |
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.
Problem
CI is failing intermittently during this test since #7196.
The test is dispatching 120 points, waiting 2 seconds, expecting to see 100 of those points written as it is capped by the max write rate, and then a final point written when the flush is called at the end of the test.
Before the change the receiver thread woke up whenever new data was received or a timeout occurred at write_frequency/2 (write_frequency is 1s in this test), while now the receiver thread wakes up every 5ms regardless of whether data is present or not. In both cases, whenever the receiver thread wakes up, it checks to see if the duration since the last write time is greater than write_frequency and logs data if it is.
Prior to the change, with the 2 second wait the thread would not log another data point prior to flush. I printed the log times and could see the wakeup prior to the flush occurs at duration since last write: 505.140834ms, meaning it is likely always taking slightly longer than 1s. This leads to two full intervals not fitting into 1s.
With the change and the 5ms wakeup, the thread does wakeup a second time, leading to an extra metrics write, causing the test to fail
Summary of Changes
Fixes #