FrankChen021 commented on code in PR #20417:
URL: https://github.com/apache/druid/pull/20417#discussion_r4104457019
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java:
##########
@@ -1437,16 +1438,17 @@ public void taskAddedOrUpdated(final TaskAnnouncement
announcement, final Worker
taskItem = tasks.get(taskId);
if (taskItem == null) {
// Try to find information about it in the TaskStorage
- Optional<TaskStatus> knownStatusInStorage =
taskStorage.getStatus(taskId);
+ final TaskInfo taskInfo = taskStorage.getTaskInfo(taskId);
Review Comment:
[P1] Keep status-only discovery non-destructive when payload decoding fails
**Finding:** TaskStorage.getTaskInfo() is nullable, and
SQLMetadataStorageActionHandler returns null when the stored payload cannot be
deserialized, such as when a task type's Jackson module is unavailable, even
though getStatus(taskId) still reports RUNNING. This branch now treats that
task as unknown, sets shouldShutdownTask for a RUNNING announcement, and kills
the worker task; the previous path tracked the known RUNNING status instead. A
metadata deserialization limitation can therefore terminate an otherwise
running ingestion task after an Overlord restart.
**Suggestion:** Distinguish an absent row from an undecodable payload and
preserve status-only tracking or another non-destructive fallback until the
full Task is available, while suppressing callbacks that require a null Task.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunnerListener.java:
##########
@@ -35,10 +36,10 @@ public interface TaskRunnerListener
* Called when the location of a task has changed. The task may not actually
be done starting up when
* this notification arrives, so it may not be listening at this location
yet.
*/
- void locationChanged(String taskId, TaskLocation newLocation);
+ void locationChanged(Task task, TaskLocation newLocation);
Review Comment:
[P1] Update every TaskRunnerListener implementation to the Task signature
**Finding:** This interface change leaves
SingleTaskBackgroundRunnerTest.java:303 implementing locationChanged(String,
TaskLocation) under @Override. With the new method descriptor, that anonymous
class no longer implements TaskRunnerListener and the indexing-service tests
fail compilation before any tests run.
**Suggestion:** Update the remaining implementations, including the second
listener in SingleTaskBackgroundRunnerTest, to accept Task and adjust their
dependent callers.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java:
##########
@@ -1437,16 +1438,17 @@ public void taskAddedOrUpdated(final TaskAnnouncement
announcement, final Worker
taskItem = tasks.get(taskId);
if (taskItem == null) {
// Try to find information about it in the TaskStorage
- Optional<TaskStatus> knownStatusInStorage =
taskStorage.getStatus(taskId);
+ final TaskInfo taskInfo = taskStorage.getTaskInfo(taskId);
Review Comment:
[P1] Update HRTR discovery tests for the new storage lookup
**Finding:** This change replaces TaskStorage.getStatus with getTaskInfo,
but HttpRemoteTaskRunnerTest.testTaskRunnerRestart (lines 451-456) and
testTaskAddedOrUpdated3 (lines 1397-1403) still configure EasyMock expectations
only for getStatus. Their first discovery callback now makes an unexpected
getTaskInfo call, so the existing indexing-service test suite fails even with
valid task payloads.
**Suggestion:** Change those fixtures to return TaskInfo objects containing
the stored Task for known tasks, retain explicit null cases for unknown tasks,
and assert the Task object delivered to listeners.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java:
##########
@@ -1437,16 +1438,17 @@ public void taskAddedOrUpdated(final TaskAnnouncement
announcement, final Worker
taskItem = tasks.get(taskId);
if (taskItem == null) {
// Try to find information about it in the TaskStorage
- Optional<TaskStatus> knownStatusInStorage =
taskStorage.getStatus(taskId);
+ final TaskInfo taskInfo = taskStorage.getTaskInfo(taskId);
Review Comment:
[P2] Avoid loading full payloads for completed worker announcements
**Finding:** getTaskInfo selects and deserializes the full task payload
before the code knows whether the stored status is RUNNING, SUCCESS, or FAILED.
Completed announcements only need the status branch, but task payloads can be
large and workers report completed tasks during startup, so a restart with many
completed or large tasks now adds avoidable metadata, CPU, and memory pressure.
**Suggestion:** Read the status first and fetch the full Task only for
RUNNING discoveries, or add a storage lookup that avoids deserializing payloads
for completed-task handling.
--
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]
