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-5211: Propagate errors from shards to coordinator #3938

Merged
merged 9 commits into from Oct 18, 2023

Conversation

raz-mon
Copy link
Collaborator

@raz-mon raz-mon commented Oct 12, 2023

Currently, the coordinator neglects errors it gets from the shards. This PR fixes this behavior, such that the coordinator will report to the user any error the shards send it.

  • Note: These are not timeout errors, which are taken care of separately here.

Mark if applicable

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

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

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

Comparison is base (b7bdc96) 82.82% compared to head (2c60b5f) 82.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3938      +/-   ##
==========================================
- Coverage   82.82%   82.80%   -0.02%     
==========================================
  Files         192      192              
  Lines       32650    32658       +8     
==========================================
+ Hits        27041    27043       +2     
- Misses       5609     5615       +6     
Files Coverage Δ
src/aggregate/aggregate_exec.c 95.76% <ø> (+0.83%) ⬆️
src/aggregate/aggregate_request.c 97.54% <ø> (ø)
src/reply.c 83.51% <ø> (-0.06%) ⬇️
coord/src/dist_aggregate.c 90.93% <88.88%> (-0.85%) ⬇️

... and 2 files with indirect coverage changes

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

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.

Nicely done!
Few comments

coord/src/dist_aggregate.c Outdated Show resolved Hide resolved
src/aggregate/aggregate_exec.c Show resolved Hide resolved
src/reply.c Show resolved Hide resolved
tests/pytests/test_resp3.py Outdated Show resolved Hide resolved
coord/src/dist_aggregate.c Outdated Show resolved Hide resolved
tests/pytests/test_coordinator.py Show resolved Hide resolved
tests/pytests/test_cursors.py Show resolved Hide resolved
@raz-mon raz-mon merged commit 52f8fa0 into master Oct 18, 2023
13 of 14 checks passed
@raz-mon raz-mon deleted the razmon-fix_coordinator_errors branch October 18, 2023 17:39
@github-actions
Copy link

Successfully created backport PR for 2.8:

@github-actions
Copy link

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

Please cherry-pick the changes locally.

git fetch origin 2.10
git worktree add -d .worktree/backport-3938-to-2.10 origin/2.10
cd .worktree/backport-3938-to-2.10
git checkout -b backport-3938-to-2.10
ancref=$(git merge-base b7bdc96723d862060dee771602f99e22c82b8043 2c60b5fa53745073730165379fcb71a7f254fbc9)
git cherry-pick -x $ancref..2c60b5fa53745073730165379fcb71a7f254fbc9

@github-actions
Copy link

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

Please cherry-pick the changes locally.

git fetch origin 2.6
git worktree add -d .worktree/backport-3938-to-2.6 origin/2.6
cd .worktree/backport-3938-to-2.6
git checkout -b backport-3938-to-2.6
ancref=$(git merge-base b7bdc96723d862060dee771602f99e22c82b8043 2c60b5fa53745073730165379fcb71a7f254fbc9)
git cherry-pick -x $ancref..2c60b5fa53745073730165379fcb71a7f254fbc9

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

3 participants