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

fix: Do not kill Dragonfly on failed DFLY LOAD #3892

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Oct 7, 2024

Today, some of the failures to load an RDB file passed via --dbfilename cause Dragonfly to terminate with an error code. This is ok and works as expected.

The problem is that the same code path is used for DFLY LOAD, which means that if there's an error loading the file (such as corrupted file), Dragonfly will exit instead of returning an error code to the client.

This change fixes that, by only exiting in the code path which loads on init.

Note: failure to load an RDB will have meant that the entire data store was flushed (like with FLUSHALL), so this is not an atomic operation.

Note to reviewer: apparently we can't call Future::Get() more than once, as the first call resets the state of the future and drops the previously saved value, so we use a Fiber here instead.

Today, some of the failures to load an RDB file passed via
`--dbfilename` cause Dragonfly to terminate with an error code. This is
ok and works as expected.

The problem is that the same code path is used for `DFLY LOAD`, which
means that if there's an error loading the file (such as corrupted
file), Dragonfly will exit instead of returning an error code to the
client.

This change fixes that, by only exiting in the code path which loads on
init.

Note to reviewer: apparently we can't call `Future::Get()` more than
once, as the first call resets the state of the future and drops the
previously saved value, so we use a Fiber here instead.
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

LGTM

@chakaz chakaz merged commit 50a7f2b into main Oct 8, 2024
12 checks passed
@chakaz chakaz deleted the chakaz/load-fail branch October 8, 2024 11:47
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