Skip to content

util/log: start garbage collection routines for old log files#68553

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
rauchenstein:log_gc
Aug 10, 2021
Merged

util/log: start garbage collection routines for old log files#68553
craig[bot] merged 1 commit intocockroachdb:masterfrom
rauchenstein:log_gc

Conversation

@rauchenstein
Copy link
Copy Markdown
Contributor

@rauchenstein rauchenstein commented Aug 6, 2021

Fixes #68259

This fixes a regression. The call the routine that deletes old log
files when max-group-size is configured was incorrectly removed in a
refactor, so that functionality was disabled. This change reinstates
it.

Release note (bug fix): File logs respect max-group-size configuration.

@rauchenstein rauchenstein requested a review from knz August 6, 2021 18:34
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Yep, this change looks right.
As we discussed I encourage you to enhance the existing file GC test to protect against regressions.

the release note could use more words though. Something like this:

"""
Release note (bug fix): File logs now respect the max-group-size configuration parameter again. This parameter had incorrectly become ignored in v21.1, resulting in arbitrarily large log directories.
"""

(release notes for bug fixes should mention when the bug was introduced and what were the symptoms of it occurring)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@rauchenstein rauchenstein force-pushed the log_gc branch 2 times, most recently from a751077 to 8547db3 Compare August 10, 2021 03:40
@rauchenstein rauchenstein requested a review from knz August 10, 2021 03:42
Copy link
Copy Markdown
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm: 💯

(modulo the fixup that the linter is asking from you)

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rauchenstein)


pkg/util/log/file_log_gc_test.go, line 162 at r2 (raw file):

	const newLogFiles = 20
	for i := 1; i < newLogFiles; i++ {
		logFn(context.Background(), fmt.Sprint(i))

why did this need to change?

@knz
Copy link
Copy Markdown
Contributor

knz commented Aug 10, 2021

after this is green, please issue the backport PR even if we're not going to merge it straight away. I'm somewhat expecting that the backport via the blathers bot won't be clean so it will be an opportunity for you to learn how to use the backport tool.

@knz
Copy link
Copy Markdown
Contributor

knz commented Aug 10, 2021

install backport via go get github.com/benesch/backport

This fixes a regression.  The call the routine that deletes old log
files when max-group-size is configured was incorrectly removed in a
refactor, so that functionality was disabled.  This change reinstates
it.

Release note (bug fix): File logs now respect the max-group-size
configuration parameter again. This parameter had incorrectly become
ignored in v21.1, resulting in arbitrarily large log directories.
@rauchenstein
Copy link
Copy Markdown
Contributor Author

bors r=knz

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 10, 2021

Build succeeded:

@craig craig bot merged commit 679ee6e into cockroachdb:master Aug 10, 2021
@knz
Copy link
Copy Markdown
Contributor

knz commented Aug 11, 2021

@rauchenstein henceforce, please be careful to add Fixes #NNN in your PR description to ensure the corresponding issue is linked properly (and auto-closed when the PR merges)

@knz
Copy link
Copy Markdown
Contributor

knz commented Aug 11, 2021

Also don't forget the backport

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.

util/log: files past the configured max-group-size are not deleted

3 participants