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

kvs: remove stats clearing #6611

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Feb 6, 2025

Problem: KVS stats clearing code was added very early on in KVS prototyping for testing. It is basically not used anymore. If we
want to test something that requires a "clean" slate for the KVS, we can just start a new instance and test with that.

Solution: Remove all KVS stats clearing code. Remove all associated tests.

Fixes #6607

@chu11 chu11 force-pushed the issue6607_kvs_stats_clear_remove branch 2 times, most recently from 399334e to 2687ac6 Compare February 8, 2025 02:14
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

Problem: Some out of date comments were left over and never
removed in the event_subscribe() function.

Remove the out of date comments.
Problem: KVS stats clearing code was added very early on in KVS
prototyping for testing.  It is basically not used anymore.  If we
want to test something that requires a "clean" slate for the KVS,
we can just start a new instance and test with that.

Solution: Remove all KVS stats clearing code.  Remove all associated
tests.

Fixes flux-framework#6607
@chu11 chu11 force-pushed the issue6607_kvs_stats_clear_remove branch from 2687ac6 to df1c9b8 Compare February 10, 2025 17:47
@mergify mergify bot merged commit 27f4390 into flux-framework:master Feb 10, 2025
35 checks passed
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.84%. Comparing base (779a41d) to head (df1c9b8).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6611      +/-   ##
==========================================
- Coverage   83.86%   83.84%   -0.03%     
==========================================
  Files         534      534              
  Lines       88381    88358      -23     
==========================================
- Hits        74124    74086      -38     
- Misses      14257    14272      +15     
Files with missing lines Coverage Δ
src/modules/kvs/kvs.c 73.96% <100.00%> (-0.23%) ⬇️
src/modules/kvs/kvstxn.c 80.26% <ø> (-0.09%) ⬇️

... and 9 files with indirect coverage changes

@chu11 chu11 deleted the issue6607_kvs_stats_clear_remove branch February 10, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvs: remove stats clearing
2 participants