MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config by kei-nan · Pull Request #5137 · RediSearch/RediSearch

kei-nan · 2024-10-30T10:23:28Z

move cursor limitation per index to be a configuration parameter
solve a bug where the coordinator would stop reading from internal cursors

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

The 128 cursor limitation is hard coded, coordinator relies on result length to determine if it is done
Move the limitation to the configuration, coordinator should only rely on the cursor id being non zero
User could update the cursor limitation, coordinator should continue reading from the internal cursors until the cursor is depleted.

Which issues this PR fixes

MOD-8009

Main objects this PR modified

Cursors

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes - changed the error message a bit

* solve a bug where the coordinator would stop reading from internal cursors

codecov · 2024-10-30T10:46:46Z

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.33%. Comparing base (2c037bc) to head (feb0b25).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/coord/dist_aggregate.c	82.35%	3 Missing ⚠️
src/coord/rmr/rmr.c	50.00%	3 Missing ⚠️
src/coord/rmr/chan.c	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5137      +/-   ##
==========================================
+ Coverage   86.30%   86.33%   +0.03%     
==========================================
  Files         190      190              
  Lines       34604    34632      +28     
==========================================
+ Hits        29864    29900      +36     
+ Misses       4740     4732       -8

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

…return value is ignored and has no real meaning.

src/config.c

src/coord/dist_aggregate.c

src/cursor.c

src/util/misc.c

src/util/misc.h

tests/ctests/test_error_parsing.c

src/cursor.c

src/config.c

Co-authored-by: GuyAv46 <[email protected]>

Co-authored-by: Omer Shadmi <[email protected]>

tests/pytests/test_cursors.py

Co-authored-by: Omer Shadmi <[email protected]>

oshadmi

Nice💪🏼

redisearch-backport-pull-request · 2024-11-05T15:09:34Z

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-5137-to-2.8 origin/2.8
cd .worktree/backport-5137-to-2.8
git switch --create backport-5137-to-2.8
git cherry-pick -x 602fe7685da6c853ce7e3ed4ff550caec58b9c07

redisearch-backport-pull-request · 2024-11-05T15:09:35Z

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-5137-to-2.6 origin/2.6
cd .worktree/backport-5137-to-2.6
git switch --create backport-5137-to-2.6
git cherry-pick -x 602fe7685da6c853ce7e3ed4ff550caec58b9c07

redisearch-backport-pull-request · 2024-11-05T15:09:36Z

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

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

git fetch origin 2.10
git worktree add -d .worktree/backport-5137-to-2.10 origin/2.10
cd .worktree/backport-5137-to-2.10
git switch --create backport-5137-to-2.10
git cherry-pick -x 602fe7685da6c853ce7e3ed4ff550caec58b9c07

…obal Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76)

redisearch-backport-pull-request · 2024-11-05T15:09:39Z

Successfully created backport PR for 8.0:

[8.0] MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config #5165

…obal Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76)

…ugh Global Config (#5165) MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76) Co-authored-by: kei-nan <[email protected]>

…ugh Global Config (#5167) * MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76) * * fix config test

…ough Global Config (#5166) MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76)

…ugh Global Config (#5168) * MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5137) * * move cursor limitation per index to be a configuration parameter * solve a bug where the coordinator would stop reading from internal cursors * * remove int return type for iterate callback functions, since their return value is ignored and has no real meaning. * * Code Review - Round #1 * * Code Review - Round #1, missing file change * * fix crash in testAggregate.py * * Code Review - Round #2 * * replace comma with a period * * if we somehow ended up with a closed channel, free the reply * Update src/config.c Co-authored-by: GuyAv46 <[email protected]> * Update src/cursor.c Co-authored-by: Omer Shadmi <[email protected]> * - fix channel test * - add logging for the broadcasting failure flow * - changed broadcast failure to be verbose instead of warning * Update tests/pytests/test_cursors.py Co-authored-by: Omer Shadmi <[email protected]> --------- Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: Omer Shadmi <[email protected]> (cherry picked from commit 602fe76) * * use RSGlobalConfig.indexCursorLimit * * apply missing cherry-pick changes * * fix netCursorCallback callback return type * * code review comments + test crash fix * * fix remaining code review comments

kei-nan added backport 2.8 backport 2.10 labels Oct 30, 2024

* move cursor limitation per index to be a configuration parameter

56d9a90

* solve a bug where the coordinator would stop reading from internal cursors

kei-nan force-pushed the master_jk_fix_cursor_limitation branch from de1ffd0 to 56d9a90 Compare October 30, 2024 10:24

kei-nan self-assigned this Oct 30, 2024

kei-nan requested a review from GuyAv46 October 30, 2024 10:27

* remove int return type for iterate callback functions, since their …

d4db9d5

…return value is ignored and has no real meaning.

GuyAv46 reviewed Oct 30, 2024

View reviewed changes

src/config.c Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

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

src/cursor.c Outdated Show resolved Hide resolved

kei-nan added 2 commits October 30, 2024 18:49

* Code Review - Round #1

3094e17

* Code Review - Round #1, missing file change

2c1ce81

kei-nan requested a review from GuyAv46 October 31, 2024 08:31

* fix crash in testAggregate.py

ce14369

GuyAv46 reviewed Oct 31, 2024

View reviewed changes

src/util/misc.c Outdated Show resolved Hide resolved

src/util/misc.c Outdated Show resolved Hide resolved

src/util/misc.h Outdated Show resolved Hide resolved

tests/ctests/test_error_parsing.c Outdated Show resolved Hide resolved

kei-nan added 2 commits October 31, 2024 11:36

* Code Review - Round #2

ebe6d3d

* replace comma with a period

0b80491

kei-nan marked this pull request as draft October 31, 2024 09:38

kei-nan marked this pull request as ready for review October 31, 2024 09:38

kei-nan requested a review from raz-mon October 31, 2024 09:39

kei-nan added the backport 8.0 label Oct 31, 2024

oshadmi added the backport 2.6 label Oct 31, 2024

oshadmi reviewed Oct 31, 2024

View reviewed changes

src/cursor.c Outdated Show resolved Hide resolved

GuyAv46 reviewed Oct 31, 2024

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

oshadmi reviewed Oct 31, 2024

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

kei-nan and others added 3 commits October 31, 2024 15:54

* if we somehow ended up with a closed channel, free the reply

94447b7

Update src/config.c

9e079aa

Co-authored-by: GuyAv46 <[email protected]>

Update src/cursor.c

2c16bd3

Co-authored-by: Omer Shadmi <[email protected]>

kei-nan requested review from GuyAv46 and oshadmi October 31, 2024 14:07

- fix channel test

6f9b6b7

kei-nan dismissed GuyAv46’s stale review via 54e62c0 November 3, 2024 14:19

kei-nan requested a review from oshadmi November 3, 2024 14:20

oshadmi reviewed Nov 3, 2024

View reviewed changes

tests/pytests/test_cursors.py Outdated Show resolved Hide resolved

Update tests/pytests/test_cursors.py

feb0b25

Co-authored-by: Omer Shadmi <[email protected]>

oshadmi approved these changes Nov 3, 2024

View reviewed changes

raz-mon approved these changes Nov 3, 2024

View reviewed changes

kei-nan added this pull request to the merge queue Nov 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2024

kei-nan added this pull request to the merge queue Nov 5, 2024

kei-nan removed this pull request from the merge queue due to a manual request Nov 5, 2024

kei-nan added this pull request to the merge queue Nov 5, 2024

Merged via the queue into master with commit 602fe76 Nov 5, 2024

kei-nan deleted the master_jk_fix_cursor_limitation branch November 5, 2024 15:09

redisearch-backport-pull-request bot mentioned this pull request Nov 5, 2024

[8.0] MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config #5165

Merged

Conversation

kei-nan commented Oct 30, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oshadmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Nov 5, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 5, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 5, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 30, 2024 •

edited

Loading