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

Format compatibility test cover compressions, including mixed #13414

Closed
wants to merge 6 commits into from

Conversation

pdillinger
Copy link
Contributor

@pdillinger pdillinger commented Feb 21, 2025

Summary: The existing format compatibility test had limited coverage of compression options, particularly newer algorithms with and without dictionary compression. There are some subtleties that need to remain consistent, such as index blocks potentially being compressed but not using the file's dictionary if they are. This involves detecting (with a rough approximation) builds with the appropriate capabilities.

The other motivation for this change is testing some potentially useful reader-side functionality that has been in place for a long time but has not been exercised until now: mixing compressions in a single SST file. The block-based SST schema puts a compression marker on each block; arguably this is for distinguishing blocks compressed using the algorithm stored in compression_name table property from blocks left uncompressed, e.g. because they did not reach the threshold of useful compression ratio, but the marker can also distinguish compression algorithms / decompressors.

As we work toward customizable compression, it seems worth unlocking the capability to leverage the existing schema and SST reader-side support for mixing compression algorithms among the blocks of a file. Yes, a custom compression could implement its own dynamic algorithm chooser with its own tag on the compressed data (e.g. first byte), but that is slightly less storage efficient and doesn't support "vanilla" RocksDB builds reading files using a mix of built-in algorithms. As a hypothetical example, we might want to switch to lz4 on a machine that is under heavy CPU load and back to zstd when load is more normal. I dug up some data indicating ~30 seconds per output file in compaction, suggesting that file-level responsiveness might be too slow. This agility is perhaps more useful with disaggregated storage, where there is more flexibility in DB storage footprint and potentially more payoff in optimizing the average footprint.

In support of this direction, I have added a backdoor capability for debug builds of ldb to generate files with a mix of compression algorithms and incorporated this into the format compatibility test. All of the existing "forward compatible" versions (currently back to 8.6) are able to read the files generated with "mixed" compression. (NOTE: there's no easy way to patch a bunch of old versions to have them support generating mixed compression files, but going forward we can auto-detect builds with this "mixed" capability.) A subtle aspect of this support that is that for proper handling of decompression contexts and digested dictionaries, we need to set the compression_name table property to zstd if any blocks are zstd compressed. I'm expecting to add better info to SST files in follow-up, but this approach here gives us forward compatibility back to 8.6.

However, in the spirit of opening things up with what makes sense under the existing schema, we only support one compression dictionary per file. It will be used by any/all algorithms that support dictionary compression. This is not outrageous because it seems standard that a dictionary is or can be arbitrary data representative of what will be compressed. This means we would need a schema change to add dictionary compression support to an existing built-in compression algorithm (because otherwise old versions and new versions would disagree on whether the data dictionary is needed with that algorithm; this could take the form of a new built-in compression type, e.g. kSnappyCompressionWithDict; only snappy, bzip2, and windows-only xpress compression lack dictionary support currently).

Looking ahead to supporting custom compression, exposing a sizeable set of CompressionTypes to the user for custom handling essentially guarantees a path for the user to put versioning on their compression even if they neglect that initially, and without resorting to managing a bunch of distinct named entities. (I'm envisioning perhaps 64 or 127 CompressionTypes open to customization, enough for ~weekly new releases with more than a year of horizon on recycling.)

More details:

  • Reduce the running time (CI cost) of the default format compatibility test by randomly sampling versions that aren't the oldest in a category. AFAIK, pretty much all regressions can be caught with the even more stripped-down SHORT_TEST.
  • Configurable make parallelism with J environment variable
  • Generate data files in a way that makes them much more eligible for index compression, e.g. bigger keys with less entropy
  • Generate enough data files
  • Remove 2.7.fb.branch from list because it shows an assertion violation when involving compression.
  • Randomly choose a contiguous subset of the compression algorithms X {dictionary, no dictionary} configuration space when generating files, with a number of files > number of algorithms. This covers all the algorithms and both dictionary/no dictionary for each release (but not in all combinations).
  • Have ldb fail if the specified compression type is not supported by the build.

Other future work needed:

  • Blob files in format compatibility test, and support for mixed compression. NOTE: the blob file schema should naturally support mixing compression algorithms but the reader code does not because of an assertion that the block CompressionType (if not no compression) matches the whole file CompressionType. We might introduce a "various" CompressionType for this whole file marker in blob files.
  • Do more to ensure certain features and code paths e.g. in the scripts are actually used in the compatibility test, so that they aren't accidentally neutralized.

Test Plan:
Manual runs with some temporary instrumentation, also a recent revision of this change included a GitHub Actions run of the updated format compatible test: https://github.com/facebook/rocksdb/actions/runs/13463551149/job/37624205915?pr=13414

@pdillinger pdillinger force-pushed the test_compression_format branch from 3fddd5e to dbbeba2 Compare February 21, 2025 19:08
@pdillinger pdillinger changed the title WIP Greatly expand compression testing in format compatibility test Feb 21, 2025
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@pdillinger pdillinger force-pushed the test_compression_format branch from dbbeba2 to e89b8b2 Compare February 21, 2025 20:35
@pdillinger pdillinger requested a review from hx235 February 21, 2025 21:32
@pdillinger pdillinger changed the title Greatly expand compression testing in format compatibility test Format compatibility test cover compressions, including mixed Feb 21, 2025
@pdillinger pdillinger marked this pull request as ready for review February 21, 2025 22:14
@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@pdillinger has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@pdillinger has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@hx235 hx235 left a comment

Choose a reason for hiding this comment

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

LGTM!

@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@pdillinger has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@pdillinger merged this pull request in 3af905a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants