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.

Sign up for GitHub

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

Jump to bottom

Allow MEMORY MALLOC-STATS and MEMORY PURGE during loading phase #1317

Open
wants to merge 5 commits into
base: unstable
Choose a base branch
from
Open

Allow MEMORY MALLOC-STATS and MEMORY PURGE during loading phase #1317

wants to merge 5 commits into from
+51 −2

Conversation

knggk
Copy link
Contributor

@knggk knggk commented Nov 18, 2024 •
edited
Loading

  • Enable investigation of memory issues during loading
  • Previously, all memory commands were rejected with LOADING error (except memory help)
  • MEMORY MALLOC-STATS and MEMORTY PURGE are now allowed as they don't depend on the dataset
  • MEMORY STATS and MEMORY USAGE KEY remain disallowed

Fixes #1299

@knggk knggk force-pushed the memory-commands-during-loading branch from 6b826d4 to 09d475d Compare November 18, 2024 22:31
@codecov Codecov
Copy link

codecov bot commented Nov 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.76%. Comparing base (aa2dd3e) to head (0c064f3).
Report is 28 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1317      +/-   ##
============================================
+ Coverage     70.73%   70.76%   +0.02%     
============================================
  Files           115      117       +2     
  Lines         63158    63315     +157     
============================================
+ Hits          44674    44802     +128     
- Misses        18484    18513      +29
Files with missing lines Coverage Δ
src/commands.def 100.00% <ø> (ø)

... and 29 files with indirect coverage changes

@hwware
Copy link
Member

hwware commented Nov 19, 2024

I would like to suggest only 3 commands except "memory stats" to be allowed during loading process because it costs a lot of resources and involves key issues. (key count, key used memory)

@madolson madolson requested a review from hpatro November 22, 2024 03:22
hpatro
hpatro reviewed Nov 22, 2024
View reviewed changes
tests/unit/loading.tcl Outdated Show resolved Hide resolved
tests/unit/loading.tcl Outdated Show resolved Hide resolved
enjoy-binbin
enjoy-binbin requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

LGTM, somehow i think we can put the test in introspection.tcl

tests/unit/loading.tcl Outdated Show resolved Hide resolved
@enjoy-binbin
Copy link
Member

please also update the top comment, we can copy the issue content instead of just linking it.

@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Nov 27, 2024
@knggk knggk changed the title Memory inspection commands no longer return loading errors Allow memory malloc-stats during loading Nov 28, 2024
@knggk knggk changed the title Allow memory malloc-stats during loading Allow memory malloc-stats and memory purge during loading phase Nov 28, 2024
@knggk
Copy link
Contributor Author

knggk commented Nov 28, 2024

please also update the top comment, we can copy the issue content instead of just linking it.

I updated the title and top comment.

@knggk
Copy link
Contributor Author

knggk commented Nov 28, 2024

LGTM, somehow i think we can put the test in introspection.tcl

Moved the test into introspection.tcl. Let's see if it passes on the CI.

@enjoy-binbin
Copy link
Member

#1368 The expire test is fixed

hwware
hwware approved these changes Nov 28, 2024
View reviewed 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.

Everything looks good, @enjoy-binbin If you agree, pls merge it.

@knggk
Copy link
Contributor Author

knggk commented Nov 30, 2024

#1368 The expire test is fixed

I merged unstable, and the tests now pass. Thank you!

enjoy-binbin
enjoy-binbin approved these changes Dec 1, 2024
View reviewed changes
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

LGTM. @valkey-io/core-team please approval

zuiderkwast
zuiderkwast approved these changes Dec 1, 2024
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Change seems good. Didn't review the diff. Is it a major decision?

madolson
madolson approved these changes Dec 1, 2024
View reviewed changes
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

@zuiderkwast I think its technically a major decision based on past decisions, since it's altering command behavior. Moving forward maybe it shouldn't be though.

@zuiderkwast zuiderkwast added the major-decision-approved Major decision approved by TSC team label Dec 2, 2024
@zuiderkwast zuiderkwast changed the title Allow memory malloc-stats and memory purge during loading phase Allow MEMORY MALLOC-STATS and MEMORY PURGE during loading phase Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwware hwware hwware approved these changes

@madolson madolson madolson approved these changes

@hpatro hpatro hpatro approved these changes

@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

@zuiderkwast zuiderkwast zuiderkwast approved these changes

Assignees
No one assigned
Labels
major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes
Projects
Valkey 8.1
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MEMORY DOCTOR/MALLOC-STATS in loading phase
6 participants
@knggk @hwware @enjoy-binbin @zuiderkwast @hpatro @madolson