Optimize poller counting #6651
Merged
Optimize poller counting #6651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of iterating through the entire history cache, use the count where the values aren't needed. Additionally remove the tasklist isolation specific logic from emitMisconfiguredPartitionMetrics. This could add additional CPU usage when enabling tasklist isolation and accurate when tasklist <-> isolation group assignment is implemented. Generally we need to revisit this metric as the source of truth for partition count is moving from the dynamic config to persistence, but this metric still relies only on dynamic config. If partition autoscaling is enabled then it seems strange to alert in this scenario.
natemort
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
shijiesheng
reviewed
Jan 30, 2025
Comment on lines
-948
to
-956
| pollerCount := len(c.pollerHistory.GetPollerInfo(time.Time{})) | ||
| if c.enableIsolation { // if isolation enabled, get the minimum poller count among the isolation groups | ||
| pollerCountsByIsolationGroup := c.pollerHistory.GetPollerIsolationGroups(time.Time{}) | ||
| for _, count := range pollerCountsByIsolationGroup { | ||
| if count < pollerCount { | ||
| pollerCount = count | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These are isolation related logic. Are they still needed?
There was a problem hiding this comment.
With the tasklist partition <-> isolation group mapping that I'm currently implementing it's no longer accurate. We've disabled isolation broadly so it's not an issue in the mean time.
taylanisikdemir
approved these changes
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of iterating through the entire history cache, use the count where the values aren't needed.
Additionally remove the tasklist isolation specific logic from emitMisconfiguredPartitionMetrics. This could add additional CPU usage when enabling tasklist isolation and is inaccurate when tasklist <-> isolation group assignment is implemented.
Generally we need to revisit this metric as the source of truth for partition count is moving from the dynamic config to persistence, but this metric still relies only on dynamic config. If partition autoscaling is enabled then it seems strange to alert in this scenario.
What changed?
len(tlMgr.pollerHistory.GetPollerInfo(time.Time{})))with getting the size of the cache.emitMisconfiguredPartitionMetrics.Why?
How did you test it?
Potential risks
Release notes
Documentation Changes