minor: Fix metric values of costBased auto-scaler#19641
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 6 of 6 changed files.
Found 1 issue: the cost-based autoscaler no longer honors the deprecated cooldown field as the documented fallback, which can change behavior for existing persisted specs.
This is an automated review by Codex GPT-5.5
| Duration.millis(this.minTriggerScaleActionFrequencyMillis) | ||
| ); | ||
| this.minScaleDownDelay = Configs.valueOrDefault(minScaleDownDelay, DEFAULT_MIN_SCALE_DELAY); | ||
| this.minScaleUpDelay = Configs.valueOrDefault(minScaleUpDelay, DEFAULT_MIN_SCALE_UP_DELAY); |
There was a problem hiding this comment.
[P2] Preserve the legacy cooldown fallback
Existing cost-based supervisor specs can still contain minTriggerScaleActionFrequencyMillis, and the shared AutoScalerConfig contract/docs say minScaleUpDelay and minScaleDownDelay fall back to it when the Duration fields are absent. This PR removes the creator parameter and always defaults minScaleUpDelay to DEFAULT_MIN_SCALE_UP_DELAY, while the getter now returns -1, so a persisted spec that only set the deprecated field is accepted but silently changes its scale-up cooldown on upgrade. Please continue binding the legacy value and using it as the fallback until the field is actually removed, or migrate the public contract/docs and persisted-spec behavior together.
| } | ||
| if (metrics.getPollIdleRatio() >= 0) { | ||
| emitter.emit(getMetricBuilder().setMetric(AVG_POLL_IDLE_RATIO, metrics.getPollIdleRatio())); | ||
| } |
There was a problem hiding this comment.
I'd also would like to request you to emit estimateIdleRatioFromProcessingRate() as separate metric for better tracking.
There was a problem hiding this comment.
change it please just to "idle ratio"
| currentTaskCount, | ||
| metrics.getAvgPartitionLag(), | ||
| metrics.getAvgProcessingRate(), | ||
| metrics.getPollIdleRatio(), |
There was a problem hiding this comment.
Call either getPollIdleRatio() or estimateIdleRatioFromProcessingRate(), whatever is active
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I reviewed the new commits since the previous review and the full current diff for correctness, edge cases, concurrency, and integration risks. No new issues found in the latest changes.
Reviewed 6 of 6 changed files.
The previously reported compatibility issue around minTriggerScaleActionFrequencyMillis remains unresolved in the current head.
This is an automated review by Codex GPT-5.5
3 participants
Changes
lagCostandidleCostmetrics (which were removed recently in minor: Add metrics to costBased auto-scaler #19631 ) but do not document themlagWeightandidleWeightavgProcessingRateandavgPollIdleRatiominTriggerScaleActionFrequency. Use default constants instead.testComputeValidTaskCountsinto multiple test casesThis PR has: