Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MOD-5990: Return timeout error from cursor on strict timeout policy #4149

Merged
merged 4 commits into from Dec 7, 2023

Conversation

raz-mon
Copy link
Collaborator

@raz-mon raz-mon commented Nov 30, 2023

Currently, we return the number of aggregated results and the results aggregated until experiencing a timeout on a cursor command, no matter the timeout policy. This PR fixes this behavior to return a timeout error (accompanied with a 0 cursor id, as before) when the timeout policy is strict (i.e., ON_TIMEOUT FAIL).

This PR also enables to return other errors from a cursor, which is a behavior we do not currently exploit. This situation is rare also for non-cursor commands, yet we want to have unified behaviors for the cursor in this regard.

(Related to #4059)

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (271d8bb) 83.37% compared to head (875a2ee) 83.36%.
Report is 2 commits behind head on master.

Files Patch % Lines
src/aggregate/aggregate_exec.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4149      +/-   ##
==========================================
- Coverage   83.37%   83.36%   -0.02%     
==========================================
  Files         191      191              
  Lines       32592    32590       -2     
==========================================
- Hits        27173    27168       -5     
- Misses       5419     5422       +3     

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

@raz-mon raz-mon requested a review from oshadmi November 30, 2023 15:12
@raz-mon raz-mon changed the title MOD-5990:Return timeout error from cursor on strict timeout policy MOD-5990: Return timeout error from cursor on strict timeout policy Nov 30, 2023
@raz-mon raz-mon force-pushed the razmon-return_error_from_cursors branch from e4755ce to 562d572 Compare December 3, 2023 14:40
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👍🏼

@raz-mon raz-mon added this pull request to the merge queue Dec 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
@raz-mon raz-mon added this pull request to the merge queue Dec 5, 2023
@raz-mon raz-mon removed this pull request from the merge queue due to a manual request Dec 6, 2023
@raz-mon raz-mon added this pull request to the merge queue Dec 6, 2023
@raz-mon raz-mon removed this pull request from the merge queue due to a manual request Dec 7, 2023
@raz-mon raz-mon added this pull request to the merge queue Dec 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 7, 2023
@raz-mon raz-mon added this pull request to the merge queue Dec 7, 2023
Merged via the queue into master with commit 89348e3 Dec 7, 2023
15 of 16 checks passed
@raz-mon raz-mon deleted the razmon-return_error_from_cursors branch December 7, 2023 13:23
Copy link

github-actions bot commented Dec 7, 2023

Successfully created backport PR for 2.8:

ephraimfeldblum pushed a commit that referenced this pull request Dec 17, 2023
…4149)

* return errors from cursor

* Update test comment

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* fix test name

* fix comment

---------

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
ephraimfeldblum added a commit that referenced this pull request Dec 17, 2023
raz-mon added a commit that referenced this pull request Mar 6, 2024
…4149)

* return errors from cursor

* Update test comment

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* fix test name

* fix comment

---------

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants