kfaraz commented on code in PR #18715:
URL: https://github.com/apache/druid/pull/18715#discussion_r2496951138
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -614,7 +614,15 @@ private void clearAllocationInfo()
partitionGroups.clear();
partitionOffsets.clear();
- pendingCompletionTaskGroups.clear();
+ // Note: We intentionally do NOT clear pendingCompletionTaskGroups here.
+ // When the autoscaler calls this method after gracefulShutdownInternal(),
tasks have been moved to
+ // pendingCompletionTaskGroups and are transitioning from READING ->
PUBLISHING state.
+ // If we clear pendingCompletionTaskGroups, the supervisor "forgets" about
these publishing tasks.
+ // Then, during the next discoverTasks(), if tasks haven't transitioned
yet, they get re-added to
+ // activelyReadingTaskGroups, causing the autoscaler to repeatedly attempt
scale-down and create
+ // duplicate supervisor history entries. By preserving
pendingCompletionTaskGroups, the autoscaler's
+ // check at DynamicAllocationTasksNotice.handle() will correctly skip
scale actions until tasks complete.
Review Comment:
Tried to make it a little concise. Also please move it to the javadoc of
this method.
```suggestion
/**
* Does not clear {@link #pendingCompletionTaskGroups} so that the
supervisor remembers that these
* tasks are publishing and auto-scaler does not repeatedly attempt a
scale down until these tasks
* complete. If this is cleared, the next {@link #discoverTasks()} might
add these tasks to
* {@link #activelyReadingTaskGroups}.
*/
```
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java:
##########
@@ -5047,6 +5050,216 @@ public void test_doesTaskMatchSupervisor()
EasyMock.replay(differentTaskType);
}
+ /**
+ * Test that clearAllocationInfo() preserves pendingCompletionTaskGroups to
prevent
+ * duplicate history entries during autoscaler scale-down operations.
+ * <p>
+ * Bug: When autoscaler scales down, it calls gracefulShutdownInternal()
which moves tasks
+ * to pendingCompletionTaskGroups, then calls clearAllocationInfo(). If
clearAllocationInfo()
+ * clears pendingCompletionTaskGroups, the supervisor "forgets" about
publishing tasks.
+ * During the next discoverTasks(), these tasks get rediscovered and
re-added to
+ * activelyReadingTaskGroups, causing the autoscaler to repeatedly attempt
scale-down
+ * and create duplicate history entries.
+ * <p>
+ * Fix: clearAllocationInfo() must preserve pendingCompletionTaskGroups so
the autoscaler's
+ * built-in check (DynamicAllocationTasksNotice.handle()) can skip scale
actions while
+ * tasks are completing.
+ */
+ @Test
+ public void test_clearAllocationInfo_preservesPendingCompletionTaskGroups()
throws Exception
Review Comment:
Rather than verifying the internal data structures of the supervisor, the
test should try to verify the behaviour of the auto-scaler.
```suggestion
public void
test_autoScaler_doesNotRepeatScaleDownActions_ifTasksAreStillPublishing()
throws Exception
```
##########
extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java:
##########
@@ -5047,6 +5050,216 @@ public void test_doesTaskMatchSupervisor()
EasyMock.replay(differentTaskType);
}
+ /**
+ * Test that clearAllocationInfo() preserves pendingCompletionTaskGroups to
prevent
+ * duplicate history entries during autoscaler scale-down operations.
+ * <p>
+ * Bug: When autoscaler scales down, it calls gracefulShutdownInternal()
which moves tasks
+ * to pendingCompletionTaskGroups, then calls clearAllocationInfo(). If
clearAllocationInfo()
+ * clears pendingCompletionTaskGroups, the supervisor "forgets" about
publishing tasks.
+ * During the next discoverTasks(), these tasks get rediscovered and
re-added to
+ * activelyReadingTaskGroups, causing the autoscaler to repeatedly attempt
scale-down
+ * and create duplicate history entries.
+ * <p>
+ * Fix: clearAllocationInfo() must preserve pendingCompletionTaskGroups so
the autoscaler's
+ * built-in check (DynamicAllocationTasksNotice.handle()) can skip scale
actions while
+ * tasks are completing.
+ */
Review Comment:
This can be omitted as it is an implementation detail.
--
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]
