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

Uses purge_all_bank_snapshots() #35380

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

brooksprumo
Copy link
Contributor

Problem

PR #35350 purges all bank snapshots after fastboot. It's implementation originally used purge_all_bank_snapshots(), a la #35291, but the impl was changed to make backporting to v1.18 easy.

This PR exists to update master back to the new function.

Summary of Changes

Use purge_all_bank_snapshots().

@brooksprumo brooksprumo self-assigned this Feb 29, 2024
@brooksprumo brooksprumo marked this pull request as ready for review February 29, 2024 21:04
@@ -344,7 +344,7 @@ fn bank_forks_from_snapshot(
// snapshot expects. This would cause the node to crash again. To prevent that, purge all
// the bank snapshots here. In the above scenario, this will cause the node to load from a
// snapshot archive next time, which is safe.
snapshot_utils::purge_old_bank_snapshots(&snapshot_config.bank_snapshots_dir, 0, None);
snapshot_utils::purge_all_bank_snapshots(&snapshot_config.bank_snapshots_dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

does it matter we don't sort now?
I think now. just confirming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, we do not need to sort. That was one of the justifications for #35291, was to remove the sort.

Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo
Copy link
Contributor Author

@apfitzge We're dog-fooding trying out requiring 2+ approvals on PRs. So I'm going to wait for your approval before merging. No rush, just letting you know.

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit 245530b into solana-labs:master Mar 1, 2024
35 checks passed
@brooksprumo brooksprumo deleted the fastboot/purge-all branch March 1, 2024 12:11
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