FrankChen021 commented on code in PR #20299:
URL: https://github.com/apache/druid/pull/20299#discussion_r4272665371
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/WorkerHolder.java:
##########
@@ -125,6 +127,7 @@ public WorkerHolder(
knownAnnouncements.forEach(e -> announcements.put(e.getTaskId(), e));
}
tasksSnapshotRef = new AtomicReference<>(announcements);
+ refreshAnnouncementsView();
Review Comment:
[P1] Avoid deriving capacity from null-resource placeholders
**Finding:** When a worker rejoins while the runner still tracks a running
task, HttpRemoteTaskRunner seeds this holder with a placeholder
TaskAnnouncement whose TaskResource is null. Because the task is RUNNING, the
eager refresh here passes it to ImmutableWorkerInfo.fromWorkerAnnouncements,
which dereferences the resource to compute capacity and throws a
NullPointerException during worker creation. The worker cannot rejoin and its
task state cannot be reconciled.
**Suggestion:** Skip null-resource placeholder announcements when building
the cached capacity view, or defer/otherwise guard this derivation until a real
worker sync supplies complete task resources.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/WorkerHolder.java:
##########
@@ -149,14 +152,31 @@ public void setBlacklistedUntil(DateTime blacklistedUntil)
public ImmutableWorkerInfo toImmutable()
{
- return ImmutableWorkerInfo.fromWorkerAnnouncements(
+ final ImmutableWorkerInfo announcementsView = announcementsViewRef.get();
+ return new ImmutableWorkerInfo(
getWorker(),
- tasksSnapshotRef.get(),
+ announcementsView.getCurrCapacityUsed(),
+ announcementsView.getCurrParallelIndexCapacityUsed(),
+ announcementsView.getCurrCapacityUsedByTaskType(),
Review Comment:
[P2] Do not share the cached mutable task-type map
**Finding:** This passes the cached view's task-type map directly into each
returned ImmutableWorkerInfo. ImmutableWorkerInfo retains non-null maps as-is
and exposes the map through getCurrCapacityUsedByTaskType(), so a caller that
mutates a returned worker snapshot also mutates the cached view; subsequent
snapshots and worker-selection decisions then see corrupted per-type counts.
Before this cache, each toImmutable() call rebuilt an independent map.
**Suggestion:** Publish an immutable copy of the task-type map in the cached
view or copy it when constructing each returned worker snapshot.
--
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]
