Skip to content

Cost-based autoscaler: allow scaling action only when metrics are available#19028

Merged
kfaraz merged 6 commits into
apache:masterfrom
Fly-Style:cba-remove-processing-rate
Feb 19, 2026
Merged

Cost-based autoscaler: allow scaling action only when metrics are available#19028
kfaraz merged 6 commits into
apache:masterfrom
Fly-Style:cba-remove-processing-rate

Conversation

@Fly-Style

@Fly-Style Fly-Style commented Feb 18, 2026

Copy link
Copy Markdown
Contributor

Default processing rate was a crutch that allowed autoscaler to start scaling actions earlier than the first metrics became available. In fact, it is confusing parameter and actually a crutch, which was removed in that patch.

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

@Fly-Style Fly-Style changed the title Remove default proc. rate; allow scaling only when metrics are available Cost-based autoscaler: allow scaling action only when metrics are available Feb 18, 2026
double pollIdleRatio
)
@Test
public void testCollectMetricsReturnsNullWhenMovingAvgRateNegative()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, the test should verify that scaling action is skipped when moving average rate is not available. The test should care less about the internal implementation details of the target and more about its overall behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was added mostly to satisfy jacoco test coverage :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😛 yay, jacoco!

Let's take the opportunity to make it a useful test then. 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion to improve test behaviour.

public int computeTaskCountForScaleAction()
{
lastKnownMetrics = collectMetrics();
if (lastKnownMetrics == null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@Fly-Style
Fly-Style requested a review from kfaraz February 18, 2026 14:18
@Fly-Style
Fly-Style force-pushed the cba-remove-processing-rate branch from 44286a6 to c69f335 Compare February 18, 2026 14:47
@kfaraz
kfaraz merged commit 68785ba into apache:master Feb 19, 2026
48 of 72 checks passed
@kfaraz

kfaraz commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Merged the PR since the failure in the docker tests is unrelated and has been seen on other PRs.
Attempt to fix the docker tests here #18849

@Fly-Style
Fly-Style deleted the cba-remove-processing-rate branch February 19, 2026 07:34
@cecemei cecemei added this to the 37.0.0 milestone Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants