-
Notifications
You must be signed in to change notification settings - Fork 5.5k
misc: Expose TotalRunningTaskCount on Coordinator #26763
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?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExpose the internal total running task count on InternalResourceGroupManager as a JMX/ODS-managed metric so it can be tracked externally for load testing and monitoring. Sequence diagram for exposing TotalRunningTaskCount via JMX/ODSsequenceDiagram
participant ODSCollector
participant JMXServer
participant InternalResourceGroupManager
participant RootInternalResourceGroup
ODSCollector->>JMXServer: query metric InternalResourceGroupManager.TotalRunningTaskCount
JMXServer->>InternalResourceGroupManager: invoke getTotalRunningTaskCount()
activate InternalResourceGroupManager
loop for each rootGroup
InternalResourceGroupManager->>RootInternalResourceGroup: getRunningTaskCount()
RootInternalResourceGroup-->>InternalResourceGroupManager: runningTaskCount
end
InternalResourceGroupManager-->>JMXServer: totalRunningTaskCount
deactivate InternalResourceGroupManager
JMXServer-->>ODSCollector: totalRunningTaskCount
Class diagram for updated InternalResourceGroupManager metric exposureclassDiagram
class InternalResourceGroupManager {
- AtomicBoolean taskLimitExceeded
- List rootGroups
+ int getTaskLimitExceeded()
+ int getTotalRunningTaskCount()
}
class RootInternalResourceGroup {
+ int getRunningTaskCount()
}
InternalResourceGroupManager --> RootInternalResourceGroup : aggregates
class Managed {
}
Managed <.. InternalResourceGroupManager : annotation on getTotalRunningTaskCount
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Exposing
getTotalRunningTaskCountvia@Managedmeans it will be called periodically by the metrics system; consider whether iterating over allrootGroupsis cheap enough or if you should cache or otherwise bound the cost of this computation. - Now that
getTotalRunningTaskCountis public, verify that access torootGroupsis thread-safe for concurrent metric reads; if not, consider synchronization or using a thread-safe collection to avoid races.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Exposing `getTotalRunningTaskCount` via `@Managed` means it will be called periodically by the metrics system; consider whether iterating over all `rootGroups` is cheap enough or if you should cache or otherwise bound the cost of this computation.
- Now that `getTotalRunningTaskCount` is public, verify that access to `rootGroups` is thread-safe for concurrent metric reads; if not, consider synchronization or using a thread-safe collection to avoid races.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary: Expose running task count so we can use ods to track how many in flight tasks a coordinator is handling. We can use this to do load testing on the coordinator. Reviewed By: prashantgolash Differential Revision: D88803334
da3f9e8 to
90fac05
Compare
|
@shelton408 |
|
Can you use SqlQueryManager.getRunningTaskCount()? |
Summary: Expose running task count so we can use it to track how many in flight tasks a coordinator is handling. Due to recent coordinator overloads/crashes, we want to use this metric to do load testing on the coordinator.
Differential Revision: D88803334
== NO RELEASE NOTE ==