Skip to content
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

Increase the HZ to make test more reliable #1680

Merged
merged 7 commits into from
Feb 7, 2025

Conversation

madolson
Copy link
Member

@madolson madolson commented Feb 6, 2025

After introducing, #1387, we saw a significant increase in other spurious wakeups because of the client cron that was added, which affected the "instantaneous eventloops per second" metric (showing it higher than before". All I did was increase the server hz to get more samples and increase the target value. This seems to work more consistently now. I also removed retries since the instantaneous value isn't dependent on number of retries.

Additionally, assert_lessthan $value [expr $retries*22000] makes no sense to me. The value is usually around 30-100us, since all it's doing is waking up and running a little bit of cron. The retries doesn't make much sense, since the retries don't impact the instantaneous value. I just removed the retries and left the 22k value for now, maybe valgrind is slow.

tests/unit/info.tcl Outdated Show resolved Hide resolved
tests/unit/info.tcl Outdated Show resolved Hide resolved
tests/unit/info.tcl Outdated Show resolved Hide resolved
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.03%. Comparing base (8d8ce19) to head (a973681).
Report is 5 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1680      +/-   ##
============================================
- Coverage     71.05%   71.03%   -0.03%     
============================================
  Files           121      121              
  Lines         65243    65254      +11     
============================================
- Hits          46357    46350       -7     
- Misses        18886    18904      +18     

see 12 files with indirect coverage changes

@madolson madolson merged commit fc55142 into valkey-io:unstable Feb 7, 2025
49 checks passed
@zuiderkwast
Copy link
Contributor

Still flaky. I got this failure right now:

*** [err]: stats: instantaneous metrics in tests/unit/info.tcl
Expected '194' to be less than '150' (context: type eval line 11 cmd {assert_lessthan $value 150} proc ::test)

https://github.com/valkey-io/valkey/actions/runs/13201529468/job/36854504972?pr=1685#step:5:6269

@madolson
Copy link
Member Author

madolson commented Feb 7, 2025

Expected '194' to be less than '150' (context: type eval line 11 cmd {assert_lessthan $value 150} proc ::test)

Must be missing something else than, because that is way higher than should be possible. EDIT: I'm totally missing something, I misunderstood how the clients cron work. The value should be less than 200.

@zuiderkwast
Copy link
Contributor

What's the formula for the clients cron? Let's add a code about it in this test case when you've figured it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants