You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2026/10/23 13:08:15 UTC
[PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
kfaraz opened a new pull request, #20706:
URL: https://github.com/apache/druid/pull/20706
### Changes
- Always consider a scale up or scale down during task rollover irrespective of the scaling cooldown.
- Since the tasks are being restarted anyway, we might as well choose a better task count.
- Reset scaling cooldown (driven by `minScaleUpDelay` / `minScaleDownDelay`) on task rollover.
- Otherwise, if the cooldown elapses soon after the task rollover has finished, we would go through another round of task restart.
- Rapid successive restarts often lead to unnecessary lag buildup and may even cause task failures in case of slow segment publish.
- The whole point of the cooldowns is to avoid such task churn.
- Minor cleanup of the `DynamicAllocationTasksNotice`
### Pending
- Add a couple of UTs for the changes in the flow
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4360632409
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -738,6 +732,20 @@ public void handle()
}
}
+ private void emitRequiredTaskCountMetric(int desiredTaskCount, @Nullable String skipReason)
+ {
+ final ServiceMetricEvent.Builder event = ServiceMetricEvent
+ .builder()
+ .setDimension(DruidMetrics.SUPERVISOR_ID, supervisorId)
+ .setDimension(DruidMetrics.DATASOURCE, dataSource)
+ .setDimension(DruidMetrics.STREAM, getIoConfig().getStream());
+ if (skipReason != null) {
+ event.setDimension(AUTOSCALER_SKIP_REASON_DIMENSION, skipReason);
+ }
+ event.setMetric(AUTOSCALER_REQUIRED_TASKS_METRIC, desiredTaskCount);
+ emitter.emit(event);
Review Comment:
Addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4365893987
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else {
+ // Reset last scale time even if taskCount is not being changed.
+ // Otherwise, we might have a scaling event right after the task rollover.
+ dynamicTriggerLastScaleRunTime = System.currentTimeMillis();
Review Comment:
Fair point. Thanks @capistrant for also calling this out.
I tend to forget the lagBased auto-scaler sometimes. š
I think in a future patch, we can try to enable task count change on rollover for `lagBased` auto-scaler as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4365469403
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else {
+ // Reset last scale time even if taskCount is not being changed.
+ // Otherwise, we might have a scaling event right after the task rollover.
+ dynamicTriggerLastScaleRunTime = System.currentTimeMillis();
Review Comment:
[P2] Do not start a cooldown when rollover count cannot be computed
**Finding:** LagBasedAutoScaler inherits SupervisorTaskAutoScaler.computeTaskCountForRollover(), which returns CANNOT_COMPUTE (-1), and CostBasedAutoScaler returns the same sentinel before metrics are available. This else branch still stamps dynamicTriggerLastScaleRunTime; with minScaleUpDelay or minScaleDownDelay longer than taskDuration, no rollover scale occurs and the next regular action is blocked for a full cooldown from rollover, even when no prior scale happened. That defeats the low-sensitivity behavior described in the PR for LagBasedAutoScaler and can leave ingestion under-resourced while lag grows.
**Suggestion:** Distinguish CANNOT_COMPUTE from a valid same-count recommendation and reset the timestamp only after a valid rollover evaluation; implement a rollover recommendation for LagBasedAutoScaler if rollover scaling is intended for both strategies.
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerTest.java:
##########
@@ -121,7 +121,7 @@ public void testReturnsUnclampedScaleInStepEvenWhenCurrentIsAboveMax()
private LagBasedAutoScaler createAutoScaler()
{
- return new LagBasedAutoScaler(mockSupervisor, config, mockSpec, mockEmitter);
+ return new LagBasedAutoScaler(mockSupervisor, config, mockSpec);
Review Comment:
[P1] Remove the stale emitter import left by this constructor change
**Finding:** After this constructor change, LagBasedAutoScalerTest no longer uses mockEmitter, but the ServiceEmitter import and field initialization remain in the test. Druid's validate Checkstyle configuration enables UnusedImports for test sources, so the indexing-service build will fail before running tests.
**Suggestion:** Delete the unused ServiceEmitter import and the mockEmitter field and initialization left behind by removing the constructor argument.
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java:
##########
@@ -4275,43 +4275,62 @@ public void testDynamicAllocationScaleUpBlockedWhenCooldownNotElapsed()
{
final long scaleUpCooldown = Duration.standardHours(1).getMillis();
final long unusedCooldown = 0L;
- final StubServiceEmitter scalingEmitter = setupSupervisorForAutoScalingTest(scaleUpCooldown, unusedCooldown, 2);
+ setupSupervisorForAutoScalingTest(scaleUpCooldown, unusedCooldown, 2);
final TestSeekableStreamSupervisor supervisor =
new StateOverrideTestSeekableStreamSupervisor(SupervisorStateManager.BasicState.RUNNING);
// First scale-up succeeds and stamps the last-scale timestamp.
- supervisor.handleDynamicAllocationTasksNotice(() -> 5, () -> {}, scalingEmitter);
+ supervisor.handleDynamicAllocationTasksNotice(() -> 5, () -> {}, false);
Assertions.assertEquals(5, supervisor.getIoConfig().getTaskCount());
// Second scale-up is within the 1h minScaleUpDelay window and must be blocked.
- supervisor.handleDynamicAllocationTasksNotice(() -> 7, () -> {}, scalingEmitter);
+ supervisor.handleDynamicAllocationTasksNotice(() -> 7, () -> {}, false);
Assertions.assertEquals(5, supervisor.getIoConfig().getTaskCount(), "Second scale-up must not take effect");
final List<ServiceMetricEvent> events =
- scalingEmitter.getMetricEvents(SeekableStreamSupervisor.AUTOSCALER_REQUIRED_TASKS_METRIC);
+ emitter.getMetricEvents(SeekableStreamSupervisor.AUTOSCALER_REQUIRED_TASKS_METRIC);
Assertions.assertEquals(2, events.size(), "Two required-tasks emissions expected (one applied, one skipped)");
// First emission: the successful scale carries no skip-reason dim and reports the applied count.
assertScaledToTaskCount(events.get(0), 5);
// Second emission: the gated scale carries the cooldown skip-reason dim and the proposed (not applied) count.
assertScaleSkipped(events.get(1), 7, "Scale cooldown not elapsed yet");
}
+ @Test
+ public void test_dynamicAllocation_allowsScaleUpDuringRollover_evenIfCooldownNotElapsed()
+ {
+ final long scaleUpCooldown = Duration.standardHours(1).getMillis();
+ final long unusedCooldown = Duration.standardHours(1).getMillis();
+ setupSupervisorForAutoScalingTest(scaleUpCooldown, unusedCooldown, 2);
+ final TestSeekableStreamSupervisor supervisor =
+ new StateOverrideTestSeekableStreamSupervisor(SupervisorStateManager.BasicState.RUNNING);
+
+ supervisor.handleDynamicAllocationTasksNotice(() -> 5, () -> {}, true);
Review Comment:
[P2] Exercise the rollover bypass with an active cooldown
**Finding:** The new rollover tests call the notice only once, while dynamicTriggerLastScaleRunTime is still zero, so a one-hour cooldown has already elapsed and the scale-up test would pass even if the bypass were removed. The scale-down test has the same gap and passes false for isRolloverInProgress. The central cooldown-bypass behavior is therefore not protected by these tests.
**Suggestion:** First perform a successful scale to stamp the cooldown, then invoke both rollover cases with isRolloverInProgress=true and assert that the second scale succeeds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "Fly-Style (via GitHub)" <gi...@apache.org>.
Fly-Style commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4366262140
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -587,39 +594,33 @@ public void handle()
long nowTime = System.currentTimeMillis();
if (spec.isSuspended()) {
log.info(
- "Skipping DynamicAllocationTasksNotice execution because [%s] supervisor is suspended",
+ "Skipping DynamicAllocationTasksNotice execution because supervisor[%s] is suspended",
supervisorId
);
return;
}
if (SupervisorStateManager.BasicState.IDLE == getState()) {
log.info(
- "Skipping DynamicAllocationTasksNotice execution because [%s] supervisor is idle",
+ "Skipping DynamicAllocationTasksNotice execution because supervisor[%s] is idle",
supervisorId
);
return;
}
- log.debug("PendingCompletionTaskGroups is [%s] for supervisor[%s] for dataSource[%s]", pendingCompletionTaskGroups,
- supervisorId,
- dataSource
+ log.debug(
Review Comment:
Still want to have this as `debug`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4359983438
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -738,6 +732,20 @@ public void handle()
}
}
+ private void emitRequiredTaskCountMetric(int desiredTaskCount, @Nullable String skipReason)
+ {
+ final ServiceMetricEvent.Builder event = ServiceMetricEvent
+ .builder()
+ .setDimension(DruidMetrics.SUPERVISOR_ID, supervisorId)
+ .setDimension(DruidMetrics.DATASOURCE, dataSource)
+ .setDimension(DruidMetrics.STREAM, getIoConfig().getStream());
+ if (skipReason != null) {
+ event.setDimension(AUTOSCALER_SKIP_REASON_DIMENSION, skipReason);
+ }
+ event.setMetric(AUTOSCALER_REQUIRED_TASKS_METRIC, desiredTaskCount);
+ emitter.emit(event);
Review Comment:
[P1] Required-count metrics use the wrong emitter
**Finding:** The new helper emits through the supervisor's emitter, but existing SeekableStreamSupervisorSpecTest cases construct LagBasedAutoScaler with a separate dynamicActionEmitter and assert required-count events there. Those events now go to spec.getEmitter() instead, so the affected tests fail and callers relying on the supplied emitter lose the metrics.
**Suggestion:** Preserve the emitter supplied to normal dynamic-allocation notices, or update the emitter contract and all existing tests and call sites consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4367167294
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else if (rolloverTaskCount != SupervisorTaskAutoScaler.CANNOT_COMPUTE) {
Review Comment:
Thanks ā I rechecked the current head. Because `LagBasedAutoScaler` inherits `CANNOT_COMPUTE`, `maybeScaleDuringTaskRollover()` leaves `dynamicTriggerLastScaleRunTime` unchanged, so regular lag-based scaling can proceed once its existing cooldown elapses. The PR description scopes rollover-only behavior to `costBased` and explicitly says `lagBased` is unaffected, so Iām withdrawing the P2 for this PR. Lag-based rollover scaling can be handled in the separate follow-up you mentioned.
Reviewed 4 of 7 changed files in this targeted follow-up.
<!-- mergelens:review -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #20706:
URL: https://github.com/apache/druid/pull/20706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4366005535
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else if (rolloverTaskCount != SupervisorTaskAutoScaler.CANNOT_COMPUTE) {
Review Comment:
I don't think this is entirely correct.
For lagBased auto-scaler with cooldown > taskDuration, we do not attempt scaling on rollover, but we do not reset the cooldown either. So, as soon as the cooldown elapses, scaling will be attempted.
As for the implementation of rollover scaling for lagBased, we can do that in a separate patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4369989918
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -587,39 +594,33 @@ public void handle()
long nowTime = System.currentTimeMillis();
if (spec.isSuspended()) {
log.info(
- "Skipping DynamicAllocationTasksNotice execution because [%s] supervisor is suspended",
+ "Skipping DynamicAllocationTasksNotice execution because supervisor[%s] is suspended",
supervisorId
);
return;
}
if (SupervisorStateManager.BasicState.IDLE == getState()) {
log.info(
- "Skipping DynamicAllocationTasksNotice execution because [%s] supervisor is idle",
+ "Skipping DynamicAllocationTasksNotice execution because supervisor[%s] is idle",
supervisorId
);
return;
}
- log.debug("PendingCompletionTaskGroups is [%s] for supervisor[%s] for dataSource[%s]", pendingCompletionTaskGroups,
- supervisorId,
- dataSource
+ log.debug(
Review Comment:
Yeah, it is okay to have this as debug for now, I feel.
If scaling is skipped due to pending groups, we have a separate info log there anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #20706:
URL: https://github.com/apache/druid/pull/20706#issuecomment-6399112629
@capistrant , I have addressed your feedback and merged the PR.
Please let me know if you had any follow up comment. I will address those in a follow up, if necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4365942684
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else if (rolloverTaskCount != SupervisorTaskAutoScaler.CANNOT_COMPUTE) {
Review Comment:
[P2] Lag-based autoscaling never participates in rollover scaling
**Finding:** LagBasedAutoScaler does not override computeTaskCountForRollover(), so it returns SupervisorTaskAutoScaler.CANNOT_COMPUTE and this branch only leaves the cooldown untouched; no rollover allocation is attempted. When minScaleUpDelay or minScaleDownDelay is longer than taskDuration, a recent regular scale therefore leaves a lag-based supervisor unable to react at the next rollover, contradicting the low-sensitivity behavior documented in the PR and allowing lag to grow until the cooldown expires.
**Suggestion:** Implement a rollover recommendation for LagBasedAutoScaler using its latest lag state, or explicitly scope and document rollover behavior to strategies that support it and add coverage for the unsupported configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #20706:
URL: https://github.com/apache/druid/pull/20706#discussion_r4366005535
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4066,17 +4072,15 @@ void maybeScaleDuringTaskRollover()
"Updating taskCount for supervisor[%s] from [%d] to [%d] during rollover.",
supervisorId, currentTaskCount, rolloverTaskCount
);
- isScalingTasksOnRollover.set(true);
- try {
- new DynamicAllocationTasksNotice(
- () -> rolloverTaskCount,
- () -> {},
- emitter
- ).handle();
- }
- finally {
- isScalingTasksOnRollover.set(false);
- }
+ handleDynamicAllocationTasksNotice(
+ () -> rolloverTaskCount,
+ () -> {},
+ true
+ );
+ } else if (rolloverTaskCount != SupervisorTaskAutoScaler.CANNOT_COMPUTE) {
Review Comment:
I don't think this is entirely correct.
For lagBased auto-scaler with cooldown > taskDuration, we do not attempt scaling on rollover, but we also do not reset the cooldown. So, as soon as the cooldown elapses, scaling will be attempted.
As for the implementation of rollover scaling for lagBased, we can do that in a separate patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] minor: Reset auto-scaling cooldown on streaming task rollover (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on PR #20706:
URL: https://github.com/apache/druid/pull/20706#issuecomment-6418107678
> @capistrant , I have addressed your feedback and merged the PR. Please let me know if you had any follow up comment. I will address those in a follow up, if necessary.
sorry I was down and out with a cold over the weekend so I didn't get a chance to look. I think it all looks good though, thanks for getting this pushed through!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org