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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
