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

storage: don't drop append futures when there's an error #14000

Merged

Conversation

andrioni
Copy link
Contributor

@andrioni andrioni commented Aug 1, 2022

Change how errors are handled while appending multiple updates at once: instead of eagerly failing and cancelling all pending futures in case one fails, wait until all of them are finished, handle successes and then failures separately.

Also return the IDs of all failed appends, instead of just selecting the first.

Motivation

  • This PR fixes a previously unreported bug: appends that failed would cause other appends to be cancelled, which in some corner cases could lead to panics due to uppers being in unexpected places.

Checklist

Change how errors are handled while appending multiple updates at once:
instead of eagerly failing and cancelling all pending futures in case
one fails, wait until all of them are finished, handle successes and
then failures separately.

Also return the IDs of all failed appends, instead of just selecting
the first.
@andrioni andrioni requested a review from aljoscha August 1, 2022 14:09
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Sweet! I had grammar nits, but I'm not sure about them myself.

src/storage/src/controller.rs Outdated Show resolved Hide resolved
src/storage/src/controller.rs Outdated Show resolved Hide resolved
andrioni and others added 2 commits August 1, 2022 17:06
Co-authored-by: Aljoscha Krettek <mail@aljoscha.dev>
Co-authored-by: Aljoscha Krettek <mail@aljoscha.dev>
@andrioni andrioni enabled auto-merge (rebase) August 1, 2022 15:06
@andrioni andrioni merged commit d21ebf0 into MaterializeInc:main Aug 1, 2022
@andrioni andrioni deleted the andrioni/please-dont-drop-futures branch August 1, 2022 16:05
@andrioni andrioni mentioned this pull request Aug 1, 2022
3 tasks
andrioni added a commit to andrioni/materialize that referenced this pull request Aug 1, 2022
andrioni added a commit to andrioni/materialize that referenced this pull request Aug 1, 2022
andrioni added a commit that referenced this pull request Aug 2, 2022
This was made unnecessary via #13902 and #14000.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants