Skip to content

feat: use zstd compressed cars as blockstores #3149

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

Merged
merged 35 commits into from
Jul 14, 2023

Conversation

aatifsyed
Copy link
Contributor

@aatifsyed aatifsyed commented Jul 7, 2023

Summary of changes

Changes introduced in this pull request:

  • new forest snapshot compress ... compresses snapshots with many zstd frames.
  • forest snapshot validate ... now supports compressed snapshots prepared by the above.

Other information and links

See also
#3085
#3074

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@aatifsyed aatifsyed force-pushed the aatifsyed/compressed-random-access-car branch 2 times, most recently from 0a8f2f3 to 3bdd327 Compare July 10, 2023 12:59
@CLAassistant
Copy link

CLAassistant commented Jul 11, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@aatifsyed aatifsyed force-pushed the aatifsyed/compressed-random-access-car branch 4 times, most recently from 71c3069 to 3348fec Compare July 12, 2023 12:18
@aatifsyed aatifsyed requested review from lemmih, elmattic and LesnyRumcajs and removed request for lemmih, LesnyRumcajs and elmattic July 12, 2023 12:18
@aatifsyed aatifsyed changed the title Aatifsyed/compressed random access car feat: use zstd compressed cars are blockstores Jul 12, 2023
@aatifsyed aatifsyed force-pushed the aatifsyed/compressed-random-access-car branch from 123eb3c to 4f01f5d Compare July 12, 2023 13:19
@aatifsyed aatifsyed marked this pull request as ready for review July 12, 2023 13:19
@lemmih lemmih self-requested a review July 12, 2023 13:21
@aatifsyed aatifsyed changed the title feat: use zstd compressed cars are blockstores feat: use zstd compressed cars as blockstores Jul 12, 2023
Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +203 to +206
assert_eq!(
unsigned_varint::io::read_usize(&[0xFD, 0x2F, 0xB5, 0x28][..]).unwrap(),
6141,
);
Copy link
Member

Choose a reason for hiding this comment

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

Does it belong in the runtime? It seems like something we could check at compile time, though this read_usize probably is not const. Perhaps it would belong more in tests if anywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The purpose of the assertion is to illustrate why we can't easily tell whether an archive is compressed or not.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add some integration tests for this subcommand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it would be great to add some bin tests under tests/

@aatifsyed aatifsyed force-pushed the aatifsyed/compressed-random-access-car branch 2 times, most recently from 18151a0 to 758e582 Compare July 13, 2023 09:53
BLS
callee
CAR/SM
CARv1/SM
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would just backquote terms like CARv1 to bypass spell checker

Copy link
Collaborator

@lemmih lemmih left a comment

Choose a reason for hiding this comment

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

Excellent PoC. Love that compressed files are faster than uncompressed.

@lemmih lemmih added this pull request to the merge queue Jul 14, 2023
Merged via the queue into main with commit 63684b7 Jul 14, 2023
@lemmih lemmih deleted the aatifsyed/compressed-random-access-car branch July 14, 2023 06:49
@aatifsyed aatifsyed mentioned this pull request Jul 18, 2023
11 tasks
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.

5 participants