-
Notifications
You must be signed in to change notification settings - Fork 162
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 support for log compression #3592
Conversation
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.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @karampok and @scrye)
go/lib/log/log.go, line 75 at r1 (raw file):
MaxAge: logAge, // days MaxBackups: logBackups, Compress: true,
That's prooobably not what you wanted to do :)
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.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @karampok and @kormat)
go/lib/log/log.go, line 75 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
That's prooobably not what you wanted to do :)
The beauty of no acceptance tests for this 🎉. Fixed :)
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.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @karampok)
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.
Reviewed 5 of 6 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @karampok)
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.
Reviewable status: complete! all files reviewed, all discussions resolved
go/lib/log/flags.go, line 31 at r2 (raw file):
logBackups int logFlush int logCompress bool
having inside log package a variable starting with logX seems redundant.
(log.logX)
go/lib/log/log.go, line 57 at r2 (raw file):
// logFlush = 0 logging output is unbuffered and Flush() is a no-op. // Set compress to true to enable rotated file compression. func SetupLogFile(name string, logDir string, logLevel string, logSize int, logAge int,
more than three arguments, should be a struct. But not important.
Another golang stylish comment
func SetupLogFile( name, dir, level string, size,age int)
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.
Reviewable status: complete! all files reviewed, all discussions resolved
go/lib/log/log.go, line 57 at r2 (raw file): Previously, karampok (Konstantinos) wrote…
This is something I was also thinking about; the refactor was unnecessarily painful due to this not taking a struct. Added to #3594. |
go/lib/log/flags.go, line 31 at r2 (raw file): Previously, karampok (Konstantinos) wrote…
This is a good point, added to #3594. |
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` Contributes scionproto#3592
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` Contributes scionproto#3592
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` Contributes scionproto#3592
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` Contributes scionproto#3592
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` The name HandlePanic was picked. Contributes #3592
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` The name HandlePanic was picked. Contributes scionproto#3592
Fixes #1924.
This change is