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

Deprecate bz2/gzip/none/tar snapshot compression types #33484

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Oct 2, 2023

Problem

For our use cases, the compression types being removed are inferior and have caused us support burden.

Summary of Changes

These options are now disallowed on the command line for solana-validator and solana-ledger-tool, which effectively means no more snapshots will be created with this types in normal usecases. However, support for reading the deprecated types is still in place.

This PR is a follow-on to #33442 (which I will be closing); that PR ripped several things out whereas this PR has a more gradual deprecation approach in mind

These options are now disallowed on the command line for
solana-validator and solana-ledger-tool, which effectively means no more
snapshots will be created with this types in normal usecases. However,
support for reading the deprecated types is still in place.
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #33484 (3c4d652) into master (3508b7d) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33484   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         802      802           
  Lines      217801   217792    -9     
=======================================
+ Hits       178114   178130   +16     
+ Misses      39687    39662   -25     

@steviez
Copy link
Contributor Author

steviez commented Oct 2, 2023

As a quick sanity check, here is ouptut running with a now deprecated value:

$ cargo run -- create-snapshot 10 --ledger ~/ledger --snapshot-archive-format none
    Finished dev [unoptimized + debuginfo] target(s) in 2.28s
     Running `.../solana/target/debug/solana-ledger-tool create-snapshot 10 --ledger ~/ledger --snapshot-archive-format none`
error: 'none' isn't a valid value for '--snapshot-archive-format <ARCHIVE_TYPE>'
	[possible values: lz4, zstd]

It is worth calling out that it will be a hard error for using old type. If we want to be even gentler, we could still accepts none, bzip2, ... and just translate those to zstd under the hood + emit a warning.

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.

Works for me. Can you confirm this does the right/expecting thing on solana-validator and solana-ledger-tool? I'm wondering what the error messages are if specifying "none", since that'll affect some current node operators when the eventually upgrade.

Edit: Hah, you beat me to it!

@brooksprumo
Copy link
Contributor

It is worth calling out that it will be a hard error for using old type. If we want to be even gentler, we could still accepts none, bzip2, ... and just translate those to zstd under the hood + emit a warning.

I'd rather hard error, personally. I'm not sure there's a valid reason to use "none" anymore, given how much faster "zstd" is. But this is not a strongly held opinion.

Minimally, we'll want this documented in release nodes/change logs.

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.

Lgtm. I'd like to have mvines review as well before merging.

@steviez steviez merged commit 73e9e6d into solana-labs:master Oct 2, 2023
33 checks passed
@steviez steviez deleted the deprecate_snap_archive_fmts branch October 2, 2023 17:40
@steviez steviez added the v1.17 PRs that should be backported to v1.17 label Oct 2, 2023
mergify bot pushed a commit that referenced this pull request Oct 2, 2023
These options are now disallowed on the command line for
solana-validator and solana-ledger-tool, which effectively means no more
snapshots will be created with this types in normal usecases. However,
support for reading the deprecated types is still in place.

(cherry picked from commit 73e9e6d)
steviez added a commit that referenced this pull request Oct 3, 2023
These options are now disallowed on the command line for
solana-validator and solana-ledger-tool, which effectively means no more
snapshots will be created with this types in normal usecases. However,
support for reading the deprecated types is still in place.

(cherry picked from commit 73e9e6d)
steviez added a commit that referenced this pull request Oct 3, 2023
…rt of #33484) (#33492)

Deprecate bz2/gzip/none/tar snapshot compression types (#33484)

These options are now disallowed on the command line for
solana-validator and solana-ledger-tool, which effectively means no more
snapshots will be created with this types in normal usecases. However,
support for reading the deprecated types is still in place.

(cherry picked from commit 73e9e6d)

Co-authored-by: steviez <steven@solana.com>
@solana-labs solana-labs locked as spam and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants