SteNicholas commented on code in PR #3858:
URL: https://github.com/apache/celeborn/pull/3858#discussion_r4424998818
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -759,6 +764,10 @@ private[celeborn] class Master(
highWorkload,
workerStatus,
requestId)
+ workerStats match {
+ case Some(stats) => workerStatsStore.update(targetWorker, stats)
Review Comment:
[P2] Coordinate heartbeat cache writes with worker removal
Master processes RPCs concurrently, so the earlier `workersMap.containsKey`
check does not guarantee the worker is still registered when this update runs.
An in-flight heartbeat can pass that check, overlap `handleWorkerLost()`
removing both the worker and its stats, then recreate the cached entry here.
Timeout scans only visit `workersMap`, so the orphaned stats can remain
indefinitely.
I reproduced this on 7617f38d by pausing a heartbeat after the registration
check, completing normal worker-loss cleanup, then resuming the heartbeat: the
worker remained absent, but its stats reappeared. This also occurs in
single-Master mode. Could we coordinate membership validation and cache writes
with removal/re-registration, and add a concurrent lifecycle regression test? A
standalone second membership check would still leave a check-then-write race.
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -843,6 +852,7 @@ private[celeborn] class Master(
s" for WorkerLost handler!")
} else {
statusSystem.handleWorkerLost(host, rpcPort, pushPort, fetchPort,
replicatePort, requestId)
+ workerStatsStore.remove(worker)
Review Comment:
[P2] Invalidate local stats across HA leadership changes
This cleanup only runs in the leader's RPC handler. If Master A caches
worker W's stats, becomes a follower, and W is lost under leader B, A applies
the loss through `MetaHandler -> updateWorkerLostMeta()` without clearing this
cache. If A becomes leader again, W's old stats remain; W is already absent
from `workersMap`, so the timeout scan cannot remove that entry.
This is the HA concern already raised in the PR discussion. A focused probe
on 7617f38d confirmed that invoking the metadata replay path removes W from
`workersMap` while retaining its stats. Could we invalidate the local cache on
leadership changes, or reconcile it with registered workers, while keeping
metrics outside Raft state? Please cover the former-leader/follower/leader
sequence with an HA regression test.
--
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]
