@redis/core-team there's still work to be done here to improve better back pressure (not just delay the client to the next event loop cycle, but possibly more, waiting for its output buffer to shrink).
But i wanted to get your feedback on the new use of the output buffer soft limit.
is that a good idea, or should we find another way to trigger this mechanism?

Have no right, just interests
I think that is a good idea. We don't increate the rick to close the client only just delay to handler the client at the next event loop cycle, and I think that also may mitigate the risk to close for reaching client output buffer.

@oranagra I think this is a good idea (especially executed fully like you describe), but it may have some undesired side effects.

For example, consider a client that does bursts of GET for large keys followed by DELs. Throttling down query buffer handling may end up delaying those deletes that would actually contribute nothing to the output buffer and may even ease up on memory.

I also agree that it makes sense.

@yoav-steinberg responding to your last comment, let's see if we can add a better back pressure.
currently the back-pressure is to postpone the next command to the next event loop, i think this by itself is in many cases very beneficial.
the only other possibilities i see are:

1. add an option for a negative soft-limit timeout which would mean that we delay the next command by x milliseconds.
2. add an option (maybe a soft-limit timeout of -1), which would mean that we don't resume commands until the output buffer is back below the soft-limit threshold.

i'm afraid that the second option would cause some (rare) cases of deadlock, in case the client won't read the responses until it's done sending the requests, so in some way the first option can be a good compromise. a very big negative value would be similar to the second option.
so that if for example you'll set it to -1000, we'll resume either after 1 second, or earlier if the buffers are again below the threshold.

WDYT?

1. add an option for a negative soft-limit timeout which would mean that we delay the next command by `x` milliseconds.

Maybe a negative time will mean back-pressure (stop processing new commands) for T time and if the COB remains above the soft limit after that time we disconnect. A positive T will leave the current implementation as is. A value of 0 will effectively be the same as the hard limit.

I've updated the concept (and the top comment).

• push-back is now until the obuf goes back to below the limit (not just to the next event loop).
• limits can be set to a percentage of the maxmemory (default is 10% for normal clients).

@redis/core-team please have a look at the top comment and tell me if you like this feature, and if you have any comments about the code or configuration.
then i'll write more tests for it and we can merge it for redis 7.0

If I understand correctly, this idea is when a client's output buffer exceeds soft limit, delay execute the pipelined command to avoid client output buffer grows.

It makes sense from the memory POV, but in some other scenarios, users are very sensitive to latency, most redis users I think. I'm very worried about that, in our product environment, the latency problem is what users mostly concern about.

Moreover, does this PR implement the monitor method? I didn't look deep into it. If we really want this feature, we need the monitor method to tell users how many time delayed by the soft limit. Because it's not easy to figure it out what impact the latency in complex product environment.

BTW, seems the old value of client-output-buffer-limit can work on the new mechanism, I think we need an explicit way to avoid users misuse it, maybe add a new config.

@soloestoy this PR is currently a bit stale, and i doubt i'll find time to promote it anytime soon, so it's likely not to make it into 7.0 anyway.
The idea behind this PR was to prevent data loss due to key eviction, but now that we have client-eviction (maxmemory-clients), this concern is somewhat covered (although not by default configuration).

you have a good point about some need for introspection (monitor) to understand what's causing latency. this PR doesn't handle it yet, but i'll add a todo for it at the top so if we'll promote this some day it'll not get forgotten.

You are right that an old value of 0 100 0 would be a valid config in the current code of the PR.
such a value (timeout of 0), is a very odd configuration, i think we can afford to change it's meaning.
i.e. IIRC in the past it would just be disconnecting a client at the next cron (not very useful), and now this just delay the client.

regarding your main concern (of causing latency):
i do think that when we proceed with this we want this to be enabled by default (unlike client-eviction), but since the main purpose of this is to cause back-pressure to prevent key-eviction, maybe we can change it to only kick in when the server approaches the maxmemory limit?
i.e. the users that are concerned about latency are probably more concerned about losing (all) their data, so that's what we should aim for to achieve by default.

any thoughts?

so it's likely not to make it into 7.0 anyway.

Agree, since we already have client-eviction, we can wait some users' feedback after 7.0 released, and then decide if we need more work on this feature.

All committers have signed the CLA.