Skip to content
This repository was archived by the owner on Oct 18, 2023. It is now read-only.

load dumps from CLI #281

Merged
merged 2 commits into from
Mar 10, 2023
Merged

load dumps from CLI #281

merged 2 commits into from
Mar 10, 2023

Conversation

MarinPostma
Copy link
Contributor

This PR enables loading a dump file from the CLI.

@MarinPostma MarinPostma requested a review from haaawk March 10, 2023 09:39
@MarinPostma MarinPostma mentioned this pull request Mar 10, 2023
Merged
athoscouto
athoscouto reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@athoscouto athoscouto left a comment

Choose a reason for hiding this comment

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

Should we prevent loading from a dump in case there is any content on the current DB?

sqld/src/lib.rs Outdated
Comment on lines 220 to 233
// load dump is necessary
let dump_loader = DumpLoader::new(config.db_path.clone(), hook.clone()).await?;
if let Some(ref path) = config.load_dump_path {
dump_loader.load_dump(path.into()).await?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only on start_primary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a replica load a dump, then there is no guarantee it's state is consistent with the primary. Hence, loading on a replica is forbidden

@MarinPostma
Copy link
Contributor Author

Should we prevent loading from a dump in case there is any content on the current DB?

I want to potentially reuse that code for HTTP later on. There is a difference between loading a dump and loading from a dump. You could, in theory, load a dump to an already running database, and if there are no conflicts this is ok. when loading from a dump, you need to start from a fresh state.

From experience, a destructive dump load is not a good solution. When loading from a dump, I can add a check that refuses to load it if there is something there already though

@MarinPostma
Copy link
Contributor Author

@athoscouto I have changed the CLI param name, and added an error when trying to load from a dump and a database already exists.

athoscouto
athoscouto approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@athoscouto athoscouto left a comment

Choose a reason for hiding this comment

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

looks good

@MarinPostma
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 10, 2023

Build succeeded:

@bors bors bot merged commit f4198be into main Mar 10, 2023
bors bot added a commit that referenced this pull request Mar 13, 2023
@bors @MarinPostma
Merge #282
d0d8a98 
282: remove dump http route r=MarinPostma a=MarinPostma

Removes the `/dump` http route, replaced with #281 


Co-authored-by: ad hoc <postma.marin@protonmail.com>
@MarinPostma MarinPostma deleted the dump-improvements branch August 25, 2023 13:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haaawk haaawk Awaiting requested review from haaawk

1 more reviewer

@athoscouto athoscouto athoscouto approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MarinPostma @athoscouto