-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
log: bump logrotate dep, switch to zstd compressor #2238
base: master
Are you sure you want to change the base?
Conversation
c57ae18
to
d977824
Compare
Needed to pick an older version of |
Pull Request Test Coverage Report for Build 10568577728Details
💛 - Coveralls |
v1.1.1 fixes an (albeit unlikely) data race. |
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.
LTM after a version bump to v1.1.
as mentioned above.
d977824
to
a7da0b2
Compare
Will update this to be a config option that defaults to the (existing) gzip before merge. |
a7da0b2
to
f6e3492
Compare
Added a config option that allows
|
f6e3492
to
6072ae2
Compare
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.
utACK, LGTM 🎉
Updated to address feedback. |
966d7dc
to
807dd5d
Compare
71baf98
to
99db92b
Compare
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.
LGTM!
99db92b
to
ceb5bfc
Compare
Rebased. |
hmmm weird CI failures... |
Oh sorry I think this is me forgetting to put in build tags for diff --git a/integration/getchaintips_test.go b/integration/getchaintips_test.go
index 1570ba74..5a4d39b5 100644
--- a/integration/getchaintips_test.go
+++ b/integration/getchaintips_test.go
@@ -1,3 +1,7 @@
+// This file is ignored during the regular tests due to the following build tag.
+//go:build rpctest
+// +build rpctest
+
package integration
import (
diff --git a/integration/invalidate_reconsider_block_test.go b/integration/invalidate_reconsider_block_test.go
index 4fe6ff00..cfc59840 100644
--- a/integration/invalidate_reconsider_block_test.go
+++ b/integration/invalidate_reconsider_block_test.go
@@ -1,3 +1,7 @@
+// This file is ignored during the regular tests due to the following build tag.
+//go:build rpctest
+// +build rpctest
+
package integration |
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.
FWIW, this ends up adding an entirely new dependency, just for slightly better log compression, how necessary is this? Seems like a nice to have, since a user can just watch the logs on disk, then rewrite them with their desired compression format.
Replacement for #2237 since
logrotate
was updated.This issue lightninglabs/taproot-assets#1084 prompted me to look at how we could switch from gzip to zstd for log compression (better compression at the default level: https://pkg.go.dev/github.com/klauspost/compress/zstd#readme-performance).
The
klauspost
ZSTD implementation seems like the best pure-Go choice, at least until it gets into the stdlib.