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

Add Snappy compression #5378

Closed
wants to merge 1 commit into from
Closed

Add Snappy compression #5378

wants to merge 1 commit into from

Conversation

meme
Copy link

@meme meme commented Oct 3, 2020

To do:

  • Remove config.h and define macros using the setup_compression.py script
  • Documentation
  • Tests

@ThomasWaldmann
Copy link
Member

Thanks for the PR!

Usually there is some discussion before adding bigger features like this, do we have a ticket about this and did you read #1633?

@ThomasWaldmann
Copy link
Member

BTW, there are some compile issues on travis-ci, see there.

@meme
Copy link
Author

meme commented Oct 4, 2020

Took a look at the issue you mentioned and I do not believe that Snappy meets all the requirements. I'll keep working on this one but it doesn't belong in Borg.

@meme meme closed this Oct 4, 2020
@ThomasWaldmann
Copy link
Member

Sorry about the work you invested. Just open an issue before starting major work next time.

Yesterday, I looked around and my impression was that snappy is somehow between zstd and lz4, but being quite close, so I didn't see a real advantage of adding it, considering we already have lz4 and zstd.

Having more code (bundled or as a dependency) always means more burden that needs to be rectified.

@meme
Copy link
Author

meme commented Oct 7, 2020

No worries about the work! The codebase is well factored so not too much time was spent at all. I hope to make more contributions to Borg in the future and will make sure to scope them out in an issue beforehand.


If I may ask: could this PR be labelled with hacktoberfest-accepted? And perhaps you may want to mark the repository with a tag for hacktoberfest as the event is now opt-in (more details here).

@ThomasWaldmann
Copy link
Member

Done, thanks for the hints.

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.

2 participants