Conversation
kei-nan
reviewed
Mar 31, 2025
kei-nan
reviewed
Mar 31, 2025
kei-nan
reviewed
Mar 31, 2025
GuyAv46
reviewed
Mar 31, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5837 +/- ##
==========================================
- Coverage 87.06% 87.01% -0.05%
==========================================
Files 211 211
Lines 37778 37788 +10
Branches 1229 1229
==========================================
- Hits 32890 32883 -7
- Misses 4883 4900 +17
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GuyAv46
previously approved these changes
Mar 31, 2025
This reverts commit 142aca0.
GuyAv46
approved these changes
Mar 31, 2025
kei-nan
approved these changes
Mar 31, 2025
Collaborator
kei-nan
left a comment
kei-nan
left a comment
There was a problem hiding this comment.
In this PR a new Next call was added which creates a very unlikely call circle:
rpSafeLoader_ResetAndReturnLastCode -> rpSafeLoaderNext_Accumulate -> rpSafeLoaderNext_Yield -> rpSafeLoader_ResetAndReturnLastCode
We reviewed the flow and it seems unlikely to happen since when setting next to yield we know we have at least one result.
Approving after reviewing the flow.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-5837-to-2.8 origin/2.8
cd .worktree/backport-5837-to-2.8
git switch --create backport-5837-to-2.8
git cherry-pick -x 7587c226ed6c27691cd9a0437a7abec3dc18c255 |
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-5837-to-2.10 origin/2.10
cd .worktree/backport-5837-to-2.10
git switch --create backport-5837-to-2.10
git cherry-pick -x 7587c226ed6c27691cd9a0437a7abec3dc18c255 |
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 8.0
git worktree add -d .worktree/backport-5837-to-8.0 origin/8.0
cd .worktree/backport-5837-to-8.0
git switch --create backport-5837-to-8.0
git cherry-pick -x 7587c226ed6c27691cd9a0437a7abec3dc18c255 |
raz-mon
added a commit
that referenced
this pull request
Apr 2, 2025
* Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0.
This was referenced Apr 2, 2025
raz-mon
added a commit
that referenced
this pull request
Apr 3, 2025
* Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 3, 2025
* MOD-9174: Fix pipeline issues (#5837) * Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0. * Fix test config params to 2.8 multi-threading
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 20, 2025
MOD-9174: Fix pipeline issues (#5837) * Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0.
JoanFM
pushed a commit
that referenced
this pull request
May 27, 2025
* Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0.
JoanFM
pushed a commit
that referenced
this pull request
May 27, 2025
* Fix pager query resultLimit setting to initialization time * Change fix to call Next in safeLoader again * Enhance comment * wip * Fix safe-loader to not deplete the pipeline on first call * Fix test * Address review * Remove all of resultLimit in Pager * Revert "Remove all of resultLimit in Pager" This reverts commit 142aca0.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Pipeline fixes
SafeLoader last result-status
The SafeLoader's last result-status mechanism was such that an
RS_RESULT_OKstatus could have been returned even though a result was not populated.This PR fixes that mechanism to continue to poll the upstream in that case, instead.
Empty result not serialized
We do not serialize empty results from the shard pipeline, as such results are not expected by the coordinator which will in turn return an error. We instead crash in development, or warn the user that an empty result has been received for serialization via a log, and skip the serialization of that result in production.
The coordinator is prepared to get a result that does not have an id in the
FT.SEARCHpipeline, in which case it will gracefully return an error instead of crashing.Assertions added
This PR adds assertions to some critical places in our code:
Healthy result id from shard
The result id exists and is not
NULLin the coordinator response parsing logic.SafeLoader last result-status
The safe-loader CANNOT return
RS_RESULT_OKupon returning the last result status, since the result it returns is not populated.