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

backupccl: fix settings registry leak during restore #67478

Merged

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Jul 12, 2021

Previously, every time a restore data processor was created, a callback
was added to the settings registry to update the size of the pool. This
could potentially lead to a large growth in registered callbacks that
close over the processors created.

This commit replaces that mechanism with a background poller that polls
the cluster setting every 30 seconds to see if the worker pool needs
updating.

Release note (bug fix): Fix a minor resource leak that occurs when a
RESTORE is run.

@pbardea pbardea requested review from a team and dt and removed request for a team July 12, 2021 13:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pbardea pbardea removed the request for review from dt July 13, 2021 15:05
Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

I don't love the singleton. If I'm reading correctly, the first settings passed is the one we'll hook up the listener to, but we went out of our way to make settings not a singleton. Tests have a tenant server -- with its settings -- and the real server running at the same time, in the same process, and we've talked about when we might have multiple tenants per process or something.

I think if we really need dynamic resize, I'd just fire up an extra goroutine that on a 5s timer reads the setting and if it changed, resize the existing sem, and pretend settings don't have callback infra at all (indeed, perhaps they shouldn't).

@pbardea pbardea force-pushed the update-restore-concurrency-throttling branch from f50ec5a to dc9e51b Compare July 16, 2021 17:24
@pbardea pbardea self-assigned this Jul 16, 2021
@pbardea pbardea force-pushed the update-restore-concurrency-throttling branch 3 times, most recently from 41a8f8b to dfa7004 Compare July 19, 2021 00:59
Previously, every time a restore data processor was created, a callback
was added to the settings registry to update the size of the pool. This
could potentially lead to a large growth in registered callbacks that
close over the processors created.

This commit replaces that mechanism with a background poller that polls
the cluster setting every 30 seconds to see if the worker pool needs
updating.

Release note (bug fix): Fix a minor resource leak that occurs when a
RESTORE is run.
@pbardea pbardea force-pushed the update-restore-concurrency-throttling branch from dfa7004 to c99a0e6 Compare July 19, 2021 02:26
@pbardea
Copy link
Contributor Author

pbardea commented Jul 19, 2021

TRTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jul 19, 2021

Build succeeded:

@craig craig bot merged commit 24825aa into cockroachdb:master Jul 19, 2021
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.

3 participants