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

fix(eviction): Tune eviction threshold in cache mode #4142

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(eviction): Tune eviction threshold in cache mode #4142

wants to merge 6 commits into from

Conversation

BagritsevichStepan
Copy link
Contributor

@BagritsevichStepan BagritsevichStepan commented Nov 18, 2024 •
edited
Loading

fixes #4139

@BagritsevichStepan BagritsevichStepan changed the title cache(cache_mode): Tune eviction threshold in cache mode fix(cache_mode): Tune eviction threshold in cache mode Nov 18, 2024
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from cdd7fc0 to b5e1611 Compare November 18, 2024 10:14
@BagritsevichStepan BagritsevichStepan changed the title fix(cache_mode): Tune eviction threshold in cache mode fix(eviction): Tune eviction threshold in cache mode Nov 18, 2024
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch 5 times, most recently from 5e6c873 to 2d7dd27 Compare November 25, 2024 10:12
@BagritsevichStepan BagritsevichStepan self-assigned this Nov 25, 2024
@BagritsevichStepan
Copy link
Contributor Author

@adiholden I believe I've identified the issue: I'm calculating rss_threshold for each shard individually, but the current rss_memory value is a global metric for all shards

@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch 3 times, most recently from 9bd58cc to fe22e03 Compare November 26, 2024 18:05
adiholden
adiholden reviewed Nov 27, 2024
View reviewed changes
tests/dragonfly/memory_test.py
await asyncio.sleep(1) # Wait for RSS heartbeat update

# First test that eviction is not triggered without connection creation
stats_info = await async_client.info("stats")
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets also add a check here that used memory is below maxmemory * 0.9 and rss curr memory is below
maxmemory * rss_oom_deny_ratio * 0.9

adiholden
adiholden reviewed Nov 27, 2024
View reviewed changes
tests/dragonfly/memory_test.py
# Increase RSS memory by 30% of max memory
# We can simulate RSS increase by creating new connections
# Estimate memory per connection
estimated_connection_memory = 15 * 1024 # 15 KB per connection
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make this test supper stable and more controlled
I would run
CONFIG SET enable_heartbeat_eviction false
before creating the connections
after that check the rss make sure its above the threshold
then run
CONFIG SET enable_heartbeat_eviction true
and after that check that rss memory dropped below the threshold and check the stats for evicted keys as you did below

@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from 0097d0a to 40d6063 Compare November 28, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiholden adiholden adiholden left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@BagritsevichStepan BagritsevichStepan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove oom_deny_ratio flag
2 participants
@BagritsevichStepan @adiholden