Add per-instance actual-state partition gauges from CurrentState - #218
Open
LZD-PratyushBhatt wants to merge 7 commits into
Open
Add per-instance actual-state partition gauges from CurrentState#218LZD-PratyushBhatt wants to merge 7 commits into
LZD-PratyushBhatt wants to merge 7 commits into
Conversation
Introduce ActualPartitionGauge and ActualTopStatePartitionGauge on the per-instance MBean, sourced from each instance's CurrentState. They complement the existing PartitionGauge and TopStatePartitionGauge, which reflect the controller's target assignment, by exposing what each instance actually hosts and how many of those partitions are in the resource top state. The counts are computed in ReadClusterDataStage in a single pass over CurrentState, alongside the existing ERROR-partition count, and routed to the instance beans through a new ClusterStatusMonitor entry point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ActualPartitionGauge counted partitions sitting in the state model's initial state (typically OFFLINE) as hosted, which inflated the gauge during bootstrap and failover. Skip both DROPPED and the initial state, matching the convention PerInstanceResourceMonitor already uses. Also harden the computation: - Move the try/catch to per-resource scope. Previously a single catch wrapped the whole loop while mutating the counter in place, so an exception midway silently emitted a partial undercount even though the javadoc promised zeros on failure. - Log a warning when a resource's state model definition cannot be resolved, instead of silently reporting zero top-state partitions. ERROR partitions are still counted there, since that state is state model agnostic. Add ClusterStatusMonitor coverage for setInstanceActualPartitionStatus (JMX values, reset to zero for instances absent from the update, and null maps), and document the per-instance gauges in Metrics.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore the defensive boundary around the live-instance lookup. That read sits inside the async cluster status update, which also refreshes every other per-instance metric, so an exception escaping here would abort the whole callable and leave all instance metrics stale rather than just zeroing one instance's partition counts. Add direct coverage for the CurrentState-derived counts, which previously had none. The tests pin the behaviour that matters: - initial-state (OFFLINE) and DROPPED partitions are not hosted - ERROR partitions are hosted, and are counted in both totals - top state is resolved per resource, so MASTER and LEADER each count against their own state model - an unresolved state model contributes no actual counts but still reports ERROR - a resource that throws is skipped without discarding counts already accumulated from other resources - non-live instances, null or empty current states, and null partition states all yield zeros instead of throwing Widen computeInstancePartitionCounts and its counts holder to package-private so the tests can exercise them, matching the existing validateAndReportInstanceDomainInfo precedent in this class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The actual partition gauge documentation described DROPPED as a state being filtered out of the hosted count, which implied participants persist it. They do not: on a successful transition to DROPPED the participant writes a SUBTRACT delta that removes the partition entry from CurrentState, so only the initial state (typically OFFLINE) and ERROR are actually observable there. Keep the DROPPED check as a cheap defensive guard, but describe it as such so the exclusion that carries real weight, the initial state, is not mistaken for a redundant one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
LZD-PratyushBhatt
requested review from
PranaviAncha,
arkmish,
kabragaurav,
laxman-ch,
ngngwr,
sjainit and
thestreak101
as code owners
August 3, 2026 06:14
LZD-PratyushBhatt
changed the title
sjainit
reviewed
Aug 13, 2026
| * Accumulate the partition counts contributed by a single resource's CurrentState into | ||
| * {@code counts}. | ||
| */ | ||
| private void accumulatePartitionCounts(BaseControllerDataProvider dataProvider, |
There was a problem hiding this comment.
why do we need to exclude "OFFLINE" from hosted partitions, since they actually live on the disk, just that they are not "SERVING" ? whats the definition of "actual partition gauge" ?
"DROPPED" makes sense since it will not occupy disk on the instance.
Collaborator
Author
There was a problem hiding this comment.
You're right. OFFLINE also means "parked on purpose" (instance, partition, or resource disabled), so blanket-excluding it made the gauge false-fire on intentional disablement. Fixed now: initial-state partitions now count when disablement is the reason
sjainit
reviewed
Aug 13, 2026
| long errorCount = countErrorPartitions(dataProvider, instanceName); | ||
| instanceErrorPartitionCounts.put(instanceName, errorCount); | ||
| // Count partitions this live instance actually hosts, from its CurrentState | ||
| InstancePartitionCounts partitionCounts = |
There was a problem hiding this comment.
will this have any performance impact on overall rebalance pipeline?
Collaborator
Author
There was a problem hiding this comment.
No impact. It runs off the pipeline thread via asyncExecute, and it's the same single pass countErrorPartitions already did, now accumulating three counters instead of one. The only new work is one in-memory getStateModelDef per resource, no extra ZK read.
sjainit
reviewed
Aug 13, 2026
| .setClusterInstanceStatus(liveInstanceSet, instanceSet, disabledInstanceSet, | ||
| disabledPartitions, oldDisabledPartitions, tags, instanceMessageMap, | ||
| instanceConfigMap, instanceErrorPartitionCounts); | ||
| clusterStatusMonitor.setInstanceActualPartitionStatus(instanceActualPartitionCounts, |
There was a problem hiding this comment.
These two monitor updates aren't atomic. setInstanceActualPartitionStatus here and the setClusterInstanceStatus call just above each acquire synchronized (_instanceMonitorMap) independently, so the pair isn't a single critical section. If registerInstances/unregisterInstances mutate _instanceMonitorMap in the window between the two calls, a live instance can momentarily publish ActualPartitionGauge/ActualTopStatePartitionGauge = 0 via the getOrDefault(instanceName, 0L) reset path, even though it is actually hosting partitions.
It is transient and self-heals on the next pipeline run, so it isn't a steady-state correctness bug — but it can surface as spurious zero dips on dashboards/alerts.
Why not fold the actual counts into setClusterInstanceStatus (passed alongside instanceErrorPartitionCounts) so everything updates under a single lock acquisition, with guaranteed ordering and no interleaving window? That also removes the implicit "must run after setClusterInstanceStatus has registered the beans" dependency this separate call currently relies on.
Collaborator
Author
There was a problem hiding this comment.
Good catch, fixed now. Both count maps now go into setClusterInstanceStatus and are applied inside the same synchronized (_instanceMonitorMap) block, and setInstanceActualPartitionStatus is deleted so the window can't be reintroduced.
The actual partition counts were published through a separate setInstanceActualPartitionStatus call made right after setClusterInstanceStatus. Each acquired the _instanceMonitorMap monitor independently, so the pair was not a single critical section. The controller async task pool has ten threads, so these updates can genuinely interleave. A bean registered by one thread could be observed by another between the two calls and reset through the getOrDefault path, letting a live instance briefly publish zero for partitions it is really hosting. It self healed on the next pipeline run, but it could surface as spurious zero dips on dashboards and alerts. Fold the counts into setClusterInstanceStatus so bean registration and gauge population happen under one lock acquisition. This also removes the implicit requirement that the separate call must run after the beans have been registered. The previous nine argument signature is kept and delegates with nulls, which now mean no information supplied and leave the gauges untouched rather than zeroing them. Also document what ActualPartitionGauge counts: state machine progress, not disk residency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review feedback noted that "actual" reads as disk residency, which invites the conclusion that an OFFLINE partition should count, since its data may well already be on the instance. The gauges measure something different: how far the instance has progressed through its state transitions. A partition can occupy disk while sitting in OFFLINE and is deliberately not counted. Rename so the name matches what is counted: ActualPartitionGauge -> ActiveStatePartitionGauge ActualTopStatePartitionGauge -> ActiveStateTopStatePartitionGauge Java identifiers, tests and Metrics.md follow the same rename. The counting semantics are unchanged, since counting the initial state would collapse the only signal these gauges provide: a live instance stuck part way through its transitions would report the same value as one that is fully caught up. Javadoc and the docs table now state explicitly that this measures state machine progress rather than disk residency. No released metric is affected, as these gauges have not shipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OFFLINE in CurrentState carries two different meanings. It can mean a partition has not finished, or has failed, its state transition, but it can equally mean the controller parked it in the initial state on purpose. The rebalancer does exactly that when the instance is disabled, when the partition is disabled on the instance, or when the whole resource is disabled. Excluding every initial-state partition conflated the two, so the gauge under-reported healthy hosts and made a naive comparison against PartitionGauge fire on intentional disablement. A partition now counts as held when it is in a non-initial state, or when it is in the initial state and one of those disablement reasons applies. Detection mirrors what BaseControllerDataProvider itself treats as disabled, so the gauge cannot drift from the rebalancer, and is resolved once per instance and once per resource rather than per partition. A missing IdealState is not read as a disabled resource, since that means the resource is being removed. A failed disablement lookup degrades to no disablement instead of dropping the instance. Also revert the gauges to their original ActualPartitionGauge and ActualTopStatePartitionGauge names, since the earlier rename was an attempt to answer this same review point by wording rather than by behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
PR Description
Issues
(Fix InstanceOperationDuration gauges to report real elapsed seconds #200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
Description
What: Adds two per-instance MBean gauges sourced from each instance's
CurrentState:ActualPartitionGaugeActualTopStatePartitionGaugeWhy: The existing
InstanceMonitor.PartitionGaugeandTopStatePartitionGaugeare derivedfrom
BestPossibleStateOutput, so they describe what the Sharding Controller intends to place onan instance. There is currently no per-instance metric for what an instance is actually hosting.
Without it, an instance that is failing to complete state transitions looks identical to a healthy
one, since the target assignment is reported either way. These gauges make the gap between intent
and reality directly observable per instance.
How: The counts are computed in
ReadClusterDataStage, in the same single pass overCurrentStatethat already produces the per-instance ERROR partition count, so no additionalZooKeeper read or extra traversal is introduced. They are routed to the instance beans through a
new
ClusterStatusMonitor#setInstanceActualPartitionStatusentry point, which also resetsinstances that are absent from the update (for example, instances that are no longer live) so that
values cannot go stale.
Counting convention, matching the one
PerInstanceResourceMonitor#updatealready uses: a partitioncounts as hosted when its state is not the state model's initial state. DROPPED is filtered
defensively only. On a successful drop the participant writes a
MergeOperation.SUBTRACTdelta thatremoves the partition entry from
CurrentStaterather than persisting DROPPED, so DROPPED is notexpected to be observable there (see
HelixStateTransitionHandler, lines 167 to 174).Resources whose state model definition cannot be resolved are skipped for the actual counts, since
their states cannot be interpreted, but ERROR is still counted because that state is state model
agnostic. Failures are contained per resource, so one unreadable resource cannot discard the counts
already accumulated for an instance.
Files changed
Tests
TestReadClusterDataStagePartitionCounts(new, 10 tests covering the count computation):testExcludesInitialStateAndDroppedPartitionstestErrorPartitionsCountAsHostedtestCountsAggregateAcrossResourcesWithDifferentStateModelstestUnresolvedStateModelSkipsActualCountsButKeepsErrorCounttestFailingResourceDoesNotDiscardOtherResourceCountstestNonLiveInstanceYieldsZeroCountstestCurrentStateReadFailureIsContainedtestNullCurrentStateMapYieldsZeroCountstestNullPartitionStateIsSkippedtestEmptyCurrentStateYieldsZeroCountsTestClusterStatusMonitor(added):testSetInstanceActualPartitionStatus, covering JMX attribute values, reset to zero forinstances absent from an update, staleness across successive updates, and null input maps
TestInstanceMonitor(added):testActualPartitionCountMetrics, covering gauge initialization, updates, and independencefrom the existing best-possible partition gauges
The following is the result of the "mvn test" command on the appropriate module:
Surrounding stages and monitors were also run to check for regressions
(
TestReadClusterDataStageDomainValidation,TestCurrentStateComputationStage,TestBestPossibleStateCalcStage,TestResourceComputationStage): 62 tests, 0 failures.The new tests were validated by temporarily reintroducing the initial-state counting behaviour and
confirming that 3 of them fail, then restoring the fix and confirming all pass. This checks that
they are genuine regression guards rather than tests that pass either way.
Changes that Break Backward Compatibility (Optional)
No breaking changes. Both gauges are new attributes on the existing
InstanceMonitorbean, andsetInstanceActualPartitionStatusis a new method. No existing metric changes value or meaning.One point reviewers should be aware of, since it affects how these metrics are used rather than
compatibility:
ActualPartitionGaugeexcludes initial-state (OFFLINE) partitions, while the existingInstanceMonitor.PartitionGaugecounts every entry in the best possible state map, includingOFFLINE and DROPPED. The two are therefore not directly subtractable. Note that
PerInstanceResourceMonitor.PartitionGauge, which shares a name with theInstanceMonitorone,already excludes both. Aligning
InstanceMonitor.PartitionGaugewith that convention would maketarget versus actual comparisons exact, but it changes an already released metric, so it is
deliberately left out of this PR.
Documentation (Optional)
In case of new functionality, my PR adds documentation in the following wiki page:
Documented in-repo rather than in the wiki:
website/1.4.3/src/site/markdown/Metrics.md, underMBean InstanceMonitor. That table previously listed none of the per-instance partition gauges, sothis adds
PartitionGauge,TopStatePartitionGauge,ActualPartitionGaugeandActualTopStatePartitionGauge.Commits
addition, my commits follow the guidelines from "How to write a good git commit message":
Code Quality
helix-style.xml(
helix-style-intellij.xmlif IntelliJ IDE is used)