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 threshold for info.tcl even higher #1687

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

madolson
Copy link
Member

@madolson madolson commented Feb 7, 2025

It's possible for the clientCron and the serverCron to get out of sync, so that even though they are both at 100hz, they could be firing independently. (e.g. server cron fires on 5, 15, 25ms and client cron fires at 0, 10, 20ms) So it's actually possible for 2x the number of crons.

@madolson madolson added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.03%. Comparing base (fc55142) to head (0e9b8ad).
Report is 1 commits behind head on unstable.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #1687   +/-   ##
=========================================
  Coverage     71.02%   71.03%           
=========================================
  Files           121      121           
  Lines         65254    65254           
=========================================
+ Hits          46344    46350    +6     
+ Misses        18910    18904    -6     

see 11 files with indirect coverage changes

Copy link
Member

@hwware hwware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense.

@hpatro hpatro merged commit 591ae9a into valkey-io:unstable Feb 7, 2025
52 of 58 checks passed
@hpatro
Copy link
Collaborator

hpatro commented Feb 7, 2025

Bunch of different failures unrelated to this PR. merged it.

@madolson
Copy link
Member Author

madolson commented Feb 7, 2025

Thanks, yeah, I ran it some more times on my private branch and I think this is the right threshold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants