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

Enable client eviction by default, set maxmemory-clients too 100% by default #1840

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

Conversation

enjoy-binbin
Copy link
Member

The client eviction introduced in 2753429,
at that time, the main idea of our introduction was to protect key data
from being evicted, which is the server's perspective.

In fact, the accumulated client output buffer can also violate the maxmemory
contract and causing the machine to OOM.

…default

The client eviction introduced in 2753429,
at that time, the main idea of our introduction was to protect key data
from being evicted, which is the server's perspective.

In fact, the accumulated client output buffer can also violate the maxmemory
contract and causing the machine to OOM.

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin added the major-decision-pending Major decision pending by TSC team label Mar 12, 2025
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.04%. Comparing base (bcd2f95) to head (ac08340).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1840      +/-   ##
============================================
+ Coverage     70.97%   71.04%   +0.06%     
============================================
  Files           123      123              
  Lines         65665    65665              
============================================
+ Hits          46608    46649      +41     
+ Misses        19057    19016      -41     
Files with missing lines Coverage Δ
src/config.c 78.77% <ø> (+0.38%) ⬆️

... and 11 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.

@hwware
Copy link
Member

hwware commented Mar 12, 2025

I am not sure if 100% as default value is reasonable

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

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

I think we shouldn't enable it by default. It cause throughput degradation (extra cpu cycle during command processing) due to constant tracking/metric update of client's input/output buffer size during read/write flow.

redis/redis#11348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants