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

Interleaved snapshot unpack versioning #27484

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Aug 30, 2022

Problem

#27346. #26590 introduced snapshot rebuilding logic that did not check version before unpacking account fields since the version file was serialized later on. We've moved the version file forward in the snapshot in #27192, so we can check the version before loading account fields.

Summary of Changes

Wait for, deserialize, and check snapshot version before processing accounts fields

Fixes #27346

@apfitzge apfitzge changed the title Issue #27346 - deserialize and check snapshot version before account fields Interleaved snapshot unpack versioning Aug 30, 2022
@apfitzge
Copy link
Contributor Author

apfitzge commented Sep 8, 2022

This shouldn't go out until #27192 is rolled out.

@apfitzge apfitzge marked this pull request as ready for review September 8, 2022 19:42
@apfitzge apfitzge requested a review from brooksprumo September 8, 2022 19:42
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Overall looks good!

@@ -199,7 +196,7 @@ struct UnarchivedSnapshot {
#[derive(Debug)]
struct UnpackedSnapshotsDirAndVersion {
unpacked_snapshots_dir: PathBuf,
snapshot_version: String,
snapshot_version: SnapshotVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

runtime/src/snapshot_utils/snapshot_storage_rebuilder.rs Outdated Show resolved Hide resolved
Comment on lines +170 to +171
match snapshot_version {
SnapshotVersion::V1_2_0 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

runtime/src/snapshot_utils/snapshot_storage_rebuilder.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils/snapshot_storage_rebuilder.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils/snapshot_storage_rebuilder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@brooksprumo brooksprumo 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 to me!

@apfitzge apfitzge merged commit a846d50 into solana-labs:master Sep 22, 2022
@apfitzge apfitzge deleted the clean_up/interleaved_snapshot_unpack_versioning_27346 branch September 22, 2022 18:56
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.

Interleaved snapshot untar/rebuilding - Versioning
2 participants