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 possibility to compress log files with Zstandard #363

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cristiano-prato
Copy link

I would like to be able to compress log files with zstandard instead of gzip.

Copy link
Collaborator

@bconn98 bconn98 left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! Can you update the compression section of the readme to include the new compression method? Also, in the Configuration docs under the docs dir. It would also be greatly appreciated if you could create a benchmark to compare the two. If you don't have the time, we can look at adding it after the fact. For a reference to a benchmark from gzip testing a while back #117.

@cristiano-prato
Copy link
Author

Hi, thanks for the feedback! I added some documentation, benchmark and proper error checking, let me know if it is ok for you!

Copy link
Collaborator

@bconn98 bconn98 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the docs + benchmark updates. Couple notes on the docs. I'm still thinking about the windows question

docs/Configuration.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cristiano-prato
Copy link
Author

I changed the documentation as suggested

Cargo.toml Outdated Show resolved Hide resolved
@estk
Copy link
Owner

estk commented Apr 1, 2024

@cristiano-prato Thanks so much for the excellent contribution. One question I had was: what happens when a user does not have zstd installed?

Other than that it lgtm

@bconn98
Copy link
Collaborator

bconn98 commented Apr 2, 2024

Agreed with @estk on that concern. I was including that in my decision process about the windows comment. Possibly a startup check with an error out?

@cristiano-prato
Copy link
Author

@cristiano-prato Thanks so much for the excellent contribution. One question I had was: what happens when a user does not have zstd installed?

Other than that it lgtm

If the zstd executable is missing, the test will fail. If you agree, I will change the test in order to use the library to decompress the archive, instead of the executable.

@cristiano-prato
Copy link
Author

I changed the unit test to avoid dependencies with zstd cli. This also allows the test to run on windows.

@cristiano-prato
Copy link
Author

Hello, is there anything I can do to improve this PR?

@bconn98
Copy link
Collaborator

bconn98 commented Jun 13, 2024

@cristiano-prato Ill take another look later today, but no I don't think so. I'm waiting to get a group of good PRs before bugging the repo owner to take a look.

@bconn98
Copy link
Collaborator

bconn98 commented Jun 18, 2024

This looks good to me. However, we need to wait for #366 & its associated PR to close first.

estk
estk previously approved these changes Jul 9, 2024
Copy link
Owner

@estk estk left a comment

Choose a reason for hiding this comment

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

This lgtm, only thing that would be nice is a usage example.

bconn98
bconn98 previously approved these changes Jul 10, 2024
@cristiano-prato cristiano-prato dismissed stale reviews from bconn98 and estk via 8c7cade July 17, 2024 10:20
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.

3 participants