MOD-8058: Avoid error with _NUM_SSTRING by oshadmi · Pull Request #5187 · RediSearch/RediSearch

oshadmi · 2024-11-10T01:03:32Z

Describe the changes in the pull request

When calling the shard with an the argument _NUM_SSTRING it is returning numeric values as errors. Need to avoid this error as it unneededly increases the error count and messes up info errorstats.

A clear and concise description of what the PR is solving, including:

Before fix:

                |       Standalone                              |           Called by Coordinator              |
                |-----------------------------------------------|----------------------------------------------|
                |   Without _NUM_SSTRING |  With _NUM_SSTRING   |   Without _NUM_SSTRING |  With _NUM_SSTRING  |
----------------|------------------------|----------------------|------------------------|---------------------|
STRING format   |   string               |  ERROR (N/A)         |   string (N/A)         |  ERROR              |
                |------------------------|----------------------|------------------------|---------------------|
EXPAND format   |                                           numeric                                            |
                |------------------------|----------------------|------------------------|---------------------|

After fix:

For RESP3 we are replying double numbers.
For RESP2 we did not modify the behavior, we reply with an Error.


                |       Standalone                              |           Called by Coordinator              |
                |-----------------------------------------------|----------------------------------------------|
                |   Without _NUM_SSTRING |  With _NUM_SSTRING   |   Without _NUM_SSTRING |  With _NUM_SSTRING  |
----------------|------------------------|----------------------|------------------------|---------------------|
STRING format   |   string               |  numeric (N/A)       |   string (N/A)         |  numeric            |
                |------------------------|----------------------|------------------------|---------------------|
EXPAND format   |                                           numeric                                            |
                |------------------------|----------------------|------------------------|---------------------|



**Which issues this PR fixes**
1. MOD-8058


**Main objects this PR modified**
1. ...
2. ...

**Mark if applicable**

- [ ] This PR introduces API changes
- [ ] This PR introduces serialization changes

codecov · 2024-11-20T15:29:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.54%. Comparing base (6d41ca3) to head (73b0fe9).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5187      +/-   ##
==========================================
- Coverage   87.56%   87.54%   -0.02%     
==========================================
  Files         196      196              
  Lines       34817    34819       +2     
==========================================
- Hits        30486    30484       -2     
- Misses       4331     4335       +4

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

raz-mon

Looking good 👍
A couple of notes:

Please add documentation in both sides of this workaround as to why we do it, i.e., in both the shard side and coordinator side.
In order to fully mitigate this bad behavior (in the error-stats only), we can try to use RESP3 only in our coordinator-shard communication. This is not an effort for this PR, but let's at least assess that effort and create a ticket for a continuation of this semi-remedy to fully solve the issue.
We can also investigate the option of using the STATUS reply type. It's a bit risky and error-prone and smells a lot like the old solution, so I'm not a fan, but it will at least not result in bad error-stats that can be highly confusing. This can be part of the continuation ticket to fully solve this, as this requires some digging in the code to make sure that it is fine (since we already reply with simple strings, which result in status reply types).
Please update the PR comment 🙂.

src/coord/dist_aggregate.c

src/value.c

MeirShpilraien · 2024-12-31T07:32:30Z

Lets specify in the top comment that it only fixes the problem if resp3 is used.

raz-mon

Nice.
Notice that this is an error-prone fix (just as the original one was), thus I think we should think of this as a dangerous patch fix and nothing more, and move on to use RESP3 in our shard-coordinator communication ASAP (CC @oshadmi @DvirDukhan).

src/coord/dist_aggregate.c

This reverts commit 06525f7.

redisearch-backport-pull-request · 2025-01-01T14:57:41Z

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-5187-to-2.8 origin/2.8
cd .worktree/backport-5187-to-2.8
git switch --create backport-5187-to-2.8
git cherry-pick -x ba6156cff8ba56c0a4415fe515b77020930870aa

redisearch-backport-pull-request · 2025-01-01T14:57:42Z

Backport failed for 2.6, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.6
git worktree add -d .worktree/backport-5187-to-2.6 origin/2.6
cd .worktree/backport-5187-to-2.6
git switch --create backport-5187-to-2.6
git cherry-pick -x ba6156cff8ba56c0a4415fe515b77020930870aa

redisearch-backport-pull-request · 2025-01-01T14:57:45Z

Successfully created backport PR for 2.10:

[2.10] MOD-8058: Avoid error with _NUM_SSTRING #5424

* Avoid error with _NUM_SSTRING * Fix logic * Enhance MRReply_ToValue to handle double conversion * Send doubles only in RESP3, errors in RESP2 * Fix RESP2 handling for double values * Revert "Fix RESP2 handling for double values" This reverts commit 06525f7. * Add comment --------- Co-authored-by: nafraf <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: raz-mon <[email protected]> (cherry picked from commit ba6156c)

redisearch-backport-pull-request · 2025-01-01T14:57:48Z

Successfully created backport PR for 8.0:

[8.0] MOD-8058: Avoid error with _NUM_SSTRING #5425

MOD-8058: Avoid error with _NUM_SSTRING (#5187) * Avoid error with _NUM_SSTRING * Fix logic * Enhance MRReply_ToValue to handle double conversion * Send doubles only in RESP3, errors in RESP2 * Fix RESP2 handling for double values * Revert "Fix RESP2 handling for double values" This reverts commit 06525f7. * Add comment --------- Co-authored-by: nafraf <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: raz-mon <[email protected]> (cherry picked from commit ba6156c) Co-authored-by: Omer Shadmi <[email protected]>

* Avoid error with _NUM_SSTRING * Fix logic * Enhance MRReply_ToValue to handle double conversion * Send doubles only in RESP3, errors in RESP2 * Fix RESP2 handling for double values * Revert "Fix RESP2 handling for double values" This reverts commit 06525f7. * Add comment --------- Co-authored-by: nafraf <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: raz-mon <[email protected]> (cherry picked from commit ba6156c)

MOD-8058: Avoid error with _NUM_SSTRING (#5187) * Avoid error with _NUM_SSTRING * Fix logic * Enhance MRReply_ToValue to handle double conversion * Send doubles only in RESP3, errors in RESP2 * Fix RESP2 handling for double values * Revert "Fix RESP2 handling for double values" This reverts commit 06525f7. * Add comment --------- Co-authored-by: nafraf <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: raz-mon <[email protected]> (cherry picked from commit ba6156c) Co-authored-by: Omer Shadmi <[email protected]>

* Avoid error with _NUM_SSTRING * Fix logic * Enhance MRReply_ToValue to handle double conversion * Send doubles only in RESP3, errors in RESP2 * Fix RESP2 handling for double values * Revert "Fix RESP2 handling for double values" This reverts commit 06525f7. * Add comment --------- Co-authored-by: nafraf <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: raz-mon <[email protected]>

This reverts commit 5b49276.

This reverts commit ef25dca. Revert "[2.8] Fix Max Frequency Misscalculation - [MOD-8158] (#5560)" This reverts commit a6864e3. Revert "[2.8] MOD-8561: Fix Inverted Index SeekTo Edge Case (#5542)" This reverts commit bb873af. Revert "[2.8] Fix legacy geofilter leak - [MOD-8568] (#5518)" This reverts commit 979f3d2. Revert "Backport 4243 to 2.8 (#5508)" This reverts commit 9af7959. Revert "MOD-7570 Backport 5422 to 2.8 (#5470)" This reverts commit 540b5f3. Revert "[2.8] MOD-7872: Add Types To Hide User Input (#5489)" This reverts commit b2bf242. Revert "[2.8] MOD-7864: Obfuscation API (#5488)" This reverts commit 4866bf1. Revert "[2.8] MOD-7634: Fix NOSTEM (#5475)" This reverts commit a015a55. Revert "[2.8] [MOD-8462] rename total_active_writes -> total_active_write_threads (#5444)" This reverts commit baadb4d. Revert "[2.8] MOD-8058: Avoid error with _NUM_SSTRING (#5187) (#5427)" This reverts commit 5b49276. Revert "Backport metrics info 2.8 [MOD-8388] (#5407)" This reverts commit f428033. Revert " [2.8] Fixes for inverted indexes encoding - [MOD-8248] (#5389)" This reverts commit 0c7227e. Revert "[2.8] MOD-8221: backport info fields (for metrics) to 2.8 (#5358)" This reverts commit b7eaa56. Revert "[2.8] Improve "Raw doc id" encoding - [MOD-8255] (#5371)" This reverts commit 152043b. Revert "[2.8] Improve SkipToBlock logic - [MOD-8255] (#5369)" This reverts commit c1a2c8f. Revert "[2.8][MOD-8115] Free spec resources in the main thread (#5324) (#5330)" This reverts commit 5f3ce64. Revert "[2.8] Fix Sorting Vector - [MOD-6783] (#5316)" This reverts commit 04f7778. Revert "[2.8] Fix a flakiness in testCursorOnCoordinator - [MOD-8016] (#5305)" This reverts commit 4fc1b27. Revert "[MOD-7949] Avoid lazy expire upon scan keys in background [2.8] (#5303)" This reverts commit 16e4ff4. Revert "[2.8] Fix long PREFIX/SUFFIX/INFIX/WILDCARD queries - [MOD-7882] (#5299)" This reverts commit 09f5b62. Revert "[2.8] Remove assertion in optimizer (#5296)" This reverts commit 45db16a. Revert "[2.8] MOD-8188: Fix OnReopen Comments (#5288)" This reverts commit 001746f. Revert "[2.8] Fix missing expansions in text queries - [MOD-8142] (#5287)" This reverts commit ed13507. Revert "[2.8] Fix Tag OnReopen Callback - [MOD-8011] (#5280)" This reverts commit 40b8012. Revert "[2.8] Fix MRIterator ownership mechanism - [MOD-8108] (#5269)" This reverts commit 90a5685. Revert "[2.8] [MOD-8125] Create inverted index in write operations only (#5260)" This reverts commit ebb9484. Revert "[2.8] MOD-8129: Fix indexed_percent info stat (#5249)" This reverts commit fb9781a. Revert "[2.8] MOD-8097, MOD-8114 Fix memory counting (#5242)" This reverts commit a21a909. Revert "[2.8] Cleanup for the trimming tree logic (#5225)" This reverts commit 1b9126b. Revert "[2.8] [MOD-8035] Add field index error for JSON field indexing failures (#5205)" This reverts commit 3a13ddb. Revert "[2.8] Fix Numeric Tree Balance - [MOD-8081, MOD-8082] (#5200)" This reverts commit a2948c4. Revert "[2.8] Performance improvement in indexBulkFields - [MOD-8093] (#5192)" This reverts commit 2b0e0a0. Revert "[2.8] MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5167)" This reverts commit f860505. Revert "[2.8] Skip test_multithread:test_async_updates_sanity with sanitizer (#5124)" This reverts commit 938369a. Revert "[2.8] Improve Error Message on VecSim Syntax Error - [MOD-7887] (#5108)" This reverts commit ba614ca. Revert "[2.8] Fix flaky test (#5084)" This reverts commit f579156. Revert "[2.8] Fix flaky test_async_updates_sanity - avoid timeout in force invoke GC [MOD-7867] (#5080)" This reverts commit 1a0eabc. Revert "[2.8] Handle VecSim in-place deletion after async [MOD-7643, MOD-7732] (#5048)" This reverts commit fcc19e9.

Avoid error with _NUM_SSTRING

8878323

oshadmi added pr:break breakingchange and removed pr:break breakingchange labels Nov 10, 2024

Fix logic

a42d269

oshadmi mentioned this pull request Dec 12, 2024

WIP: Modify MRReply_ToValue to handle double conversion #5341

Closed

2 tasks

nafraf and others added 2 commits December 12, 2024 06:25

Merge branch 'master' into oshadmi_mod-8058

e8ea3c7

Enhance MRReply_ToValue to handle double conversion

f92bd54

nafraf added the action:run-benchmark label Dec 12, 2024

Merge branch 'master' into oshadmi_mod-8058

51ad3fa

oshadmi requested a review from nafraf December 17, 2024 17:22

Merge branch 'master' into oshadmi_mod-8058

ea0a195

nafraf mentioned this pull request Dec 29, 2024

MOD-8578: Use fast_float library #5349

Merged

2 tasks

Send doubles only in RESP3, errors in RESP2

07ba740

raz-mon reviewed Dec 31, 2024

View reviewed changes

src/coord/dist_aggregate.c Outdated Show resolved Hide resolved

src/value.c Show resolved Hide resolved

Fix RESP2 handling for double values

06525f7

raz-mon reviewed Jan 1, 2025

View reviewed changes

src/coord/dist_aggregate.c Outdated Show resolved Hide resolved

src/coord/dist_aggregate.c Outdated Show resolved Hide resolved

src/coord/dist_aggregate.c Outdated Show resolved Hide resolved

kei-nan reviewed Jan 1, 2025

View reviewed changes

src/coord/dist_aggregate.c Show resolved Hide resolved

raz-mon added 2 commits January 1, 2025 15:14

Revert "Fix RESP2 handling for double values"

f28b921

This reverts commit 06525f7.

Add comment

73b0fe9

raz-mon approved these changes Jan 1, 2025

View reviewed changes

kei-nan approved these changes Jan 1, 2025

View reviewed changes

raz-mon enabled auto-merge January 1, 2025 13:26

raz-mon added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels Jan 1, 2025

raz-mon added this pull request to the merge queue Jan 1, 2025

Merged via the queue into master with commit ba6156c Jan 1, 2025
41 of 43 checks passed

raz-mon deleted the oshadmi_mod-8058 branch January 1, 2025 14:57

redisearch-backport-pull-request bot mentioned this pull request Jan 1, 2025

[2.10] MOD-8058: Avoid error with _NUM_SSTRING #5424

Merged

redisearch-backport-pull-request bot mentioned this pull request Jan 1, 2025

[8.0] MOD-8058: Avoid error with _NUM_SSTRING #5425

Merged

This was referenced Jan 1, 2025

[2.8] MOD-8058: Avoid error with _NUM_SSTRING (#5187) #5427

Merged

[2.6] MOD-8058: Avoid error with _NUM_SSTRING #5428

Closed

raz-mon removed the backport 2.6 label Jan 1, 2025

kei-nan added a commit that referenced this pull request Jan 30, 2025

Revert "[2.8] MOD-8058: Avoid error with _NUM_SSTRING (#5187) (#5427)"

463133c

This reverts commit 5b49276.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MOD-8058: Avoid error with _NUM_SSTRING#5187

MOD-8058: Avoid error with _NUM_SSTRING#5187
raz-mon merged 10 commits intomasterfrom
oshadmi_mod-8058

oshadmi commented Nov 10, 2024 •

edited by nafraf

Loading

Uh oh!

codecov bot commented Nov 20, 2024 •

edited

Loading

Uh oh!

raz-mon left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

MeirShpilraien commented Dec 31, 2024

Uh oh!

raz-mon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

oshadmi commented Nov 10, 2024 • edited by nafraf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

raz-mon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MeirShpilraien commented Dec 31, 2024

Uh oh!

raz-mon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

redisearch-backport-pull-request bot commented Jan 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

oshadmi commented Nov 10, 2024 •

edited by nafraf

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

raz-mon left a comment •

edited

Loading