-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
CompressLog: Add zstd compression #1400
Conversation
b47d3f1
to
3c798cd
Compare
See my comments @ nixpkgs. |
b20b98e
to
ec1c5a3
Compare
ec1c5a3
to
ec27939
Compare
Looks good to me, but haven't tested. Would really love it if a small test is added, but maybe that's a big ask. Would definitely improve my confidence, at least of the case where CompressLog is called. The fallback is probably a bit more annoying to test. |
ec27939
to
55a4f7e
Compare
I have tested this on my hydra and from what I can tell, it seems to work minus the bit that the original logs where not removed. I don't have time to write a test especially with the current test infrastructure. |
21fb3f0
to
e1db234
Compare
e1db234
to
aa6234a
Compare
I did some more testing and the logs where not displayed in the web ui. That is now also fixed. I think I have now tested this change end to end and hopefully found all the misses along the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff now looks good to me, unfortunately I cannot test it this week. Maybe next week.
Out of curiosity, what happens if you fill in something else than bz2 or zstd? It'll just fail somewhere in the compression routines?
I hope so. 😅 I didn't test this explicitly. |
4d2326a
to
b2b2d6e
Compare
Basically #1219 rebased on master