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

Add active_rehashing_cpu_milliseconds to INFO STATS #13110

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

enjoy-binbin
Copy link
Collaborator

This field is similar to expire_cycle_cpu_milliseconds, it represents
the CPU time we use in activerehashing, to see how much time we generally
spend in activerehashing.

This field is similar to expire_cycle_cpu_milliseconds, it represents
the CPU time we use in activerehashing, to see how much time we generally
spend in activerehashing.
@enjoy-binbin enjoy-binbin marked this pull request as ready for review March 5, 2024 11:23
@enjoy-binbin enjoy-binbin requested a review from a team March 5, 2024 11:23
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

LGTM. @redis/core-team please approve.

@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Mar 11, 2024
@oranagra
Copy link
Member

we discussed this in a core-team meeting and we weren't sure if we want it, and how useful it is.
considering we do have the activerehashing config, one can argue that this information is useful to take actions, but on the other hand, there are other info fields that indicate there's a rehashing in progress, and we're not sure knowing the amount of CPU utilized is really helpful.

we didn't want to spam the info fields (although this is a mirror of expire_cycle_cpu_milliseconds)
we decided to hold this aside until someone comes with good indication that it's really needed and how it'll be used.

@oranagra oranagra removed the approval-needed Waiting for core team approval to be merged label Mar 13, 2024
@enjoy-binbin
Copy link
Collaborator Author

another point i want to mention is that currently we will spend a fixed 1000us in each serverCron to do activerehash. The text describe it in redis.conf is:

The default is to use this millisecond 10 times every second in order to actively rehash the main dictionaries, freeing memory when possible.

however, this is when hz = 10. Sometimes users set hz randomly, causing the load to become higher. although we now have some new indicators, it doesn’t seem to be clear (we did not mention it in the redis.conf, it look like we should). or i previously suggested changing 1000us to something like active expire or defrag based on hz calculation.

but on the other hand, there are other info fields that indicate there's a rehashing in progress, and we're not sure knowing the amount of CPU utilized is really helpful.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
All committers have signed the CLA.

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

Successfully merging this pull request may close these issues.

None yet

3 participants