RM_Call - only enforce OOM on scripts if 'M' flag is sent #11425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjpotter
changed the title
Currently, RM_Call enforces OOM on scripts (if script is not flagged with ALLOW_OOM) in all cases, if the "M" flag is sent for OOM checking or if its not. This PR modifies the flow to treat apply ALLOW_OOM flag to the client if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call)
sjpotter
force-pushed
the
rm_call_oom_scripts
branch
from
a19dfc8
to
1e09bb9
Compare
oranagra
added
state:major-decision
oranagra
reviewed
Oct 24, 2022
* re-order first set of tests so they are more in-line with the tests before the PR (smaller diff) * make two disctinct groups, one with and one without M flag * remove excessive tests that checks that no-writes doesn't allow writes but does allow reads * improve comments
This was referenced Oct 27, 2022
Mixficsol
pushed a commit
to Mixficsol/redis
that referenced
this pull request
Apr 12, 2023
RM_Call is designed to let modules call redis commands disregarding the OOM state (the module is responsible to declare its command flags to redis, or perform the necessary checks). The other (new) alternative is to pass the "M" flag to RM_Call so that redis can OOM reject commands implicitly. However, Currently, RM_Call enforces OOM on scripts (excluding scripts that declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present. This PR fixes scripts to be consistent with other commands being executed by RM_Call. It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call, so no OOM checking should be done on script). Co-authored-by: Oran Agra <oran@redislabs.com>
madolson
pushed a commit
to madolson/redis
that referenced
this pull request
Apr 19, 2023
RM_Call is designed to let modules call redis commands disregarding the OOM state (the module is responsible to declare its command flags to redis, or perform the necessary checks). The other (new) alternative is to pass the "M" flag to RM_Call so that redis can OOM reject commands implicitly. However, Currently, RM_Call enforces OOM on scripts (excluding scripts that declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present. This PR fixes scripts to be consistent with other commands being executed by RM_Call. It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call, so no OOM checking should be done on script). Co-authored-by: Oran Agra <oran@redislabs.com>
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
RM_Call is designed to let modules call redis commands disregarding the OOM state (the module is responsible to declare its command flags to redis, or perform the necessary checks). The other (new) alternative is to pass the "M" flag to RM_Call so that redis can OOM reject commands implicitly. However, Currently, RM_Call enforces OOM on scripts (excluding scripts that declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present. This PR fixes scripts to be consistent with other commands being executed by RM_Call. It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call, so no OOM checking should be done on script). Co-authored-by: Oran Agra <oran@redislabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RM_Call is designed to let modules call redis commands disregarding the OOM state (the module is responsible to declare its command flags to redis, or perform the necessary checks).
The other (new) alternative is to pass the "M" flag to RM_Call so that redis can OOM reject commands implicitly.
However, Currently, RM_Call enforces OOM on scripts (excluding scripts that declared
allow-oom) in all cases, regardless of the RM_Call "M" flag being present.This PR fixes scripts to be consistent with other commands being executed by RM_Call.
It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call, so no OOM checking should be done on script).
This replaced #11161 instead of requiring a new flag, it redefines how RM_call treats the M flag (or the lack of M flag).