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

util/log: add GC of old log files #13083

Merged
merged 1 commit into from Jan 23, 2017

Conversation

petermattis
Copy link
Collaborator

@petermattis petermattis commented Jan 23, 2017

GC old log files, controlled by the MaxFilesPerSeverity setting (default
20).


This change is Reviewable

@petermattis
Copy link
Collaborator Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


pkg/util/log/file.go, line 45 at r1 (raw file):

// MaxFilesPerSeverity is the maximum number of log files per severity.
var MaxFilesPerSeverity = 20

Should this be MaxSizePerSeverity? With the current settings, we could conceivably have 20 10MB log files per severity for a total of 1000MB of log files. That's quite a bit, though not horrific if the user a 1TB+ size disk.


Comments from Reviewable

@bdarnell
Copy link
Member

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/util/log/clog_test.go, line 540 at r1 (raw file):

	MaxSize = 1 // ensure rotation on every log write

	for i := 0; i < 100; i++ {

s/100/MaxFilesPerSeverity*10/


pkg/util/log/file.go, line 45 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Should this be MaxSizePerSeverity? With the current settings, we could conceivably have 20 10MB log files per severity for a total of 1000MB of log files. That's quite a bit, though not horrific if the user a 1TB+ size disk.

Yeah, limiting by size would be better, I think.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


pkg/util/log/clog_test.go, line 540 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

s/100/MaxFilesPerSeverity*10/

Done.


pkg/util/log/file.go, line 45 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yeah, limiting by size would be better, I think.

Done.


Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/gc-log-files branch 3 times, most recently from 89eb9fc to d0a3f27 Compare January 23, 2017 21:02
GC old log files, controlled by the MaxSizePerSeverity setting (default
MaxSize*10).

Fixes cockroachdb#13040
@petermattis petermattis merged commit ceed8e3 into cockroachdb:master Jan 23, 2017
@petermattis petermattis deleted the pmattis/gc-log-files branch January 23, 2017 21:26
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.

None yet

2 participants