-
Couldn't load subscription status.
- Fork 3.7k
[fix][broker] Run ResourceGroup tasks only when tenants/namespaces registered #24859
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
base: master
Are you sure you want to change the base?
[fix][broker] Run ResourceGroup tasks only when tenants/namespaces registered #24859
Conversation
…idle ResourceGroupService previously executed periodic tasks unconditionally every resourceUsageTransportPublishIntervalInSecs. This change starts both tasks only when at least one tenant or namespace is attached to any resource group, and stops them when no attachments remain. Each task reschedules itself if the interval changes at runtime. close() now cancels and clears both futures. Fixes apache#24693 Signed-off-by: Vinkal Chudgar <vinkal.chudgar@gmail.com>
Signed-off-by: Vinkal Chudgar <vinkal.chudgar@gmail.com>
Signed-off-by: Vinkal Chudgar <vinkal.chudgar@gmail.com>
|
@vinkal-chudgar Please add the following content to your PR description and select a checkbox: |
|
Thanks for the great contribution! I didn't do a full review yet. A few minor comments. |
Thanks for the quick look! I'll address the comments when available. |
|
@lhotari - When convenient, could you please review this PR? |
| if (!schedulersRunning.get() || resourceGroupsMap.isEmpty()) { | ||
| return; | ||
| } | ||
| if (tenantToRGsMap.isEmpty() && namespaceToRGsMap.isEmpty()) { |
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.
Would it make sense to have a method hasActiveResourceGroups() for avoiding duplication of the logic?
| this.aggregateLocalUsagePeriodInSeconds = this.resourceUsagePublishPeriodInSeconds = periodInSecs; | ||
| this.aggregateLocalUsagePeriodicTask = this.pulsar.getExecutor().scheduleAtFixedRate( | ||
| // True if at least one tenant or namespace is attached to any RG. | ||
| private boolean hasActiveAttachments() { |
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.
Is "attachment" an existing concept? Just wondering if a resource group could be considered "active" when it has at least one usage? Therefore the name could be hasActiveResourceGroups etc ?
Motivation
Fixes #24693.
ResourceGroupService unconditionally schedules two periodic tasks (aggregating local usage and calculating quotas) during broker initialization, executing them every resourceUsageTransportPublishIntervalInSecs (default 60 seconds) regardless of whether any tenants or namespaces are registered to resource groups. This wastes resources on brokers where the resource group feature is unused.
This PR implements explicit lifecycle management: periodic tasks now start only when the first tenant or namespace is registered to any resource group, and stop when the last registration is removed. This eliminates unnecessary periodic task execution and resource consumption on brokers where the resource group feature is idle.
Modifications
Modified files
Verifying this change
This change is verified by unit tests. Tests are sleep free, run with per test isolation, and use 60s timeouts.
New unit tests (ResourceGroupServiceTest.java)
Updated unit tests (ResourceGroupServiceTest.java)
testClose(): start schedulers via an attachment, call close(), then assert scheduled task references are set to null and isSchedulersRunning() is false.
testResourceGroupOps(): The test now calls
calculateQuotaForAllResourceGroups()before unregistering all attachments, rather than after.Test utilities
Personal CI Results
Tested in Personal CI fork: vinkal-chudgar#1
Status:
Evidence: https://github.com/vinkal-chudgar/pulsar/pull/1/checks
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Configuration note: Only the documentation string for
resourceUsageTransportPublishIntervalInSecswas updated. No default values were changed.Documentation
docdoc-requireddoc-not-neededdoc-completeNote: Inline configuration documentation was updated in ServiceConfiguration.java. No website docs need changes
Matching PR in forked repository
PR in forked repository: vinkal-chudgar#1