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

feat: Track world state db versions and wipe the state upon version change #9946

Merged
merged 14 commits into from
Nov 19, 2024

Conversation

PhilWindle
Copy link
Collaborator

Implements a very basic mechanism of tracking changes to the world state DB structure and deleting the world state when a change is detected.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

LGTM, I'd change the file contents though to make them readable.

Aside from that, and not for this PR, should we add a "delete if incompatible" flag that defaults to false, so a configuration issue does not lead to the whole db being nuked?

Comment on lines 19 to 21
public async writeVersionFile(filename: string) {
await writeFile(filename, this.toBuffer());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use a string representation instead, so the file is human-readable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree

@PhilWindle
Copy link
Collaborator Author

LGTM, I'd change the file contents though to make them readable.

Aside from that, and not for this PR, should we add a "delete if incompatible" flag that defaults to false, so a configuration issue does not lead to the whole db being nuked?

I'm wondering if actually we shouldn't delete the DB at all. Just log a big ERROR and quit the application. Let the user do the deleting? What do you think @alexghr @spalladino?

@spalladino
Copy link
Collaborator

I'm wondering if actually we shouldn't delete the DB at all. Just log a big ERROR and quit the application. Let the user do the deleting?

Honestly, today I'd go with whatever is easier for the spartan deployments we're running. Given we're still working with very ephemeral networks, maybe we do want to delete it for convenience.

@alexghr
Copy link
Contributor

alexghr commented Nov 14, 2024

I'm wondering if actually we shouldn't delete the DB at all. Just log a big ERROR and quit the application. Let the user do the deleting? What do you think @alexghr @spalladino?

On the one hand, I would prefer this (principle of least surprise), on the other hand, in k8s how convenient is it for people to manually log into their production cluster to delete a volume to restart their node?

@PhilWindle PhilWindle enabled auto-merge (squash) November 15, 2024 09:48
@PhilWindle PhilWindle merged commit 209d484 into master Nov 19, 2024
68 checks passed
@PhilWindle PhilWindle deleted the pw/add-db-versioning branch November 19, 2024 16:16
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.

3 participants