Skip to content

Add latency stats around cluster config file operations#1534

Merged
enjoy-binbin merged 2 commits into
valkey-io:unstablefrom
enjoy-binbin:cluster_config_latency
Jan 11, 2025
Merged

Add latency stats around cluster config file operations#1534
enjoy-binbin merged 2 commits into
valkey-io:unstablefrom
enjoy-binbin:cluster_config_latency

Conversation

@enjoy-binbin

@enjoy-binbin enjoy-binbin commented Jan 9, 2025

Copy link
Copy Markdown
Member

When the cluster changes, we need to persist the cluster configuration,
and these file IO operations may cause latency.

@enjoy-binbin enjoy-binbin mentioned this pull request Jan 9, 2025
Merged
@codecov

codecov Bot commented Jan 9, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.95%. Comparing base (b207b42) to head (8efc449).
⚠️ Report is 493 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 84.61% 4 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1534      +/-   ##
============================================
+ Coverage     70.92%   70.95%   +0.03%     
============================================
  Files           120      120              
  Lines         65004    65055      +51     
============================================
+ Hits          46104    46162      +58     
+ Misses        18900    18893       -7
Files with missing lines Coverage Δ
src/cluster_legacy.c 87.14% <84.61%> (+0.44%) ⬆️

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@enjoy-binbin
Add latency around cluster config file save
523f1e3 
When the cluster changes, we need to persist the cluster configuration,
and these file IO operations may cause latency.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin force-pushed the cluster_config_latency branch from 33047ce to 523f1e3 Compare January 9, 2025 04:39
madolson
madolson reviewed Jan 9, 2025
View reviewed changes
Comment thread src/cluster_legacy.c
@madolson madolson added the release-notes This issue should get a line item in the release notes label Jan 9, 2025
@hpatro hpatro changed the title Add latency around cluster config file save Add latency stats around cluster config file operations Jan 9, 2025
hpatro
hpatro approved these changes Jan 9, 2025
View reviewed changes

@hpatro hpatro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also add some sanity tests?

@enjoy-binbin

enjoy-binbin commented Jan 10, 2025

Copy link
Copy Markdown
Member Author 
start_cluster 1 1 {tags {"latency-monitor cluster external:skip needs:latency"} overrides {latency-monitor-threshold 1}} {
    test "Cluster config file latency" {
        puts [r latency latest]
    }
}

i can get this ouput in my local mac, but it is not certain that they will show in CI. Do you have a better idea?

{command 1736483355 1 1} {cluster-config-fsync 1736483355 6 6} {cluster-config-rename 1736483355 1 1} {cluster-config-open 1736483355 1 1} {cluster-config-close 1736483355 1 1} {fork 1736483355 1 1} {cluster-config-dir-fsync 1736483355 5 5}
[ok]: Cluster config file latency (1 ms)

@hpatro

hpatro commented Jan 10, 2025

Copy link
Copy Markdown
Contributor

i can get this ouput in my local mac, but it is not certain that they will show in CI. Do you have a better idea?

Didn't follow. Could you explain why it won't show up in CI?

@enjoy-binbin

enjoy-binbin commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Didn't follow. Could you explain why it won't show up in CI?

If it is 0ms, it will not show up in the tests.

@enjoy-binbin enjoy-binbin force-pushed the cluster_config_latency branch from 5e6bda7 to 671a100 Compare January 11, 2025 03:01
@enjoy-binbin
Add tests
8efc449 
Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin force-pushed the cluster_config_latency branch from 671a100 to 8efc449 Compare January 11, 2025 03:01
@enjoy-binbin enjoy-binbin merged commit 11cb8ee into valkey-io:unstable Jan 11, 2025
@enjoy-binbin enjoy-binbin deleted the cluster_config_latency branch January 11, 2025 03:03
proost pushed a commit to proost/valkey that referenced this pull request Jan 17, 2025
@enjoy-binbin @proost
Add latency stats around cluster config file operations (valkey-io#1534)
e8e6f68 
When the cluster changes, we need to persist the cluster configuration,
and these file IO operations may cause latency.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: proost <jwalag87@gmail.com>
kronwerk pushed a commit to kronwerk/valkey that referenced this pull request Jan 27, 2025
@enjoy-binbin @kronwerk
Add latency stats around cluster config file operations (valkey-io#1534)
52357ed 
When the cluster changes, we need to persist the cluster configuration,
and these file IO operations may cause latency.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin mentioned this pull request Aug 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PingXie PingXie PingXie approved these changes
@hpatro hpatro hpatro approved these changes
@madolson madolson madolson approved these changes

Assignees

No one assigned

Labels

cluster release-notes This issue should get a line item in the release notes

Projects

Valkey 8.1
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@enjoy-binbin @hpatro @PingXie @madolson