Skip to content

[MOD-11398] Add "ignore" option to ON_OOM config#6828

Merged
lerman25 merged 4 commits intomasterfrom
omerl-add-ignore-on-oom-config
Sep 17, 2025
Merged

[MOD-11398] Add "ignore" option to ON_OOM config#6828
lerman25 merged 4 commits intomasterfrom
omerl-add-ignore-on-oom-config

Conversation

@lerman25
Copy link
Collaborator

@lerman25 lerman25 commented Sep 15, 2025

Add ignore as option to ON_OOM config.

Because on_oom and on_timeout do not share policy options, the changes from #6769 are reverted and the policy enums are seperated.

Because of the revert, I suggest, for an easier review, to review by commit and not the PR as is, the revert commit was done automaticly.

@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.98%. Comparing base (7ddefed) to head (b742bd4).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/aggregate/aggregate_exec.c 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6828      +/-   ##
==========================================
- Coverage   87.20%   86.98%   -0.23%     
==========================================
  Files         288      288              
  Lines       46050    46174     +124     
  Branches     8954     9072     +118     
==========================================
+ Hits        40159    40163       +4     
- Misses       5740     5860     +120     
  Partials      151      151              
Flag Coverage Δ
flow 84.68% <98.18%> (-0.13%) ⬇️
unit 49.70% <14.54%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@GuyAv46 GuyAv46 left a comment

Choose a reason for hiding this comment

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

Very nice

OomPolicy_Return, // Return what we have on OOM
OomPolicy_Fail, // Just fail without returning anything
OomPolicy_Ignore, // Ignore OOM and continue
OomPolicy_Invalid // Not a real value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the Invalid value? (regarding both enums)

Copy link
Collaborator Author

@lerman25 lerman25 Sep 16, 2025

Choose a reason for hiding this comment

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

In timeout, it is used in SET ON_TIMEOUT calling TimeoutPolicy_Parse, if a user gave a wrong string as option it returns TimeoutPolicy_Invalid.
I followed suit.
You think it's better if we remove it and use a different return value for parse?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, up to you. I think it's OK, especially if it's aligned with past configs. I don't think it adds anything, and we don't store invalid configs, so it should be unreachable

@lerman25 lerman25 added this pull request to the merge queue Sep 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 16, 2025
@lerman25 lerman25 added this pull request to the merge queue Sep 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 16, 2025
@lerman25 lerman25 added this pull request to the merge queue Sep 17, 2025
Merged via the queue into master with commit 33b07f8 Sep 17, 2025
18 checks passed
@lerman25 lerman25 deleted the omerl-add-ignore-on-oom-config branch September 17, 2025 06:55
lerman25 added a commit that referenced this pull request Sep 24, 2025
* Revert "[MOD-11161] Add ON_OOM config (#6769)"
This reverts commit 345beec.

* Re-Add ON_OOM config - with ignore

* cover enums

* Refactor timeout and OOM policy string conversion to use assertions for validity
YaacovHazan pushed a commit to redis/redis that referenced this pull request Oct 27, 2025
(RediSearch/RediSearch#7076,
RediSearch/RediSearch#6857) - Introducing
`FT.HYBRID`, a new command that enables hybrid queries combining both
text and vector search, with support for **RRF** and **LINEAR** result
combination. This update enhances performance and reliability through a
more efficient networking layer, smoother query execution, and improved
overall stability.
(RediSearch/RediSearch#7065) - Add
`search-default-scorer` configuration to set the default text scorer
across queries (by default it is BM25).
RediSearch/RediSearch#7022 - Handle Atomic Slot
Migration events upon moving slots from one node to another in a cluster
mode.
(RediSearch/RediSearch#6769,
RediSearch/RediSearch#6828,
RediSearch/RediSearch#6877,
RediSearch/RediSearch#6921) - Introduce query
time memory guardrails by adding a new `search-on-oom` configuration
which defines the query engine behavior when OOM (Out Of Memory) is
reached. OOM checks are applied to `FT.SEARCH`, `FT.AGGREGATE`, and
`FT.HYBRID` commands. The behavior on OOM can be configured to one of
three modes: `IGNORE`, `FAIL`, or `RETURN`.
`IGNORE` - The default behavior, run queries anyway (not recommended for
heavy queries returning a large result set).
`FAIL` - Fail query execution immediately if any of the nodes are in OOM
state when query execution starts.
`RETURN` - A best effort appraoch to return partial results when OOM is
detected in only some of the nodes in a cluster mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants