zaynt4606 commented on code in PR #3832:
URL: https://github.com/apache/celeborn/pull/3832#discussion_r3892567695
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -935,6 +935,19 @@ private[celeborn] class Worker(
sb.toString()
}
+ override def healthCheck(): HandleResponse = {
+ val state = workerStatusManager.currentWorkerStatus.getState
+ if (!registered.get()) {
Review Comment:
Could we clear `registered` as soon as a heartbeat response explicitly
reports `registered == false`, before calling `registerWithMaster()`? This flag
is only set to `true` after a successful registration. As written, `/health`
can continue returning 200 while re-registration retries, which can last up to
`celeborn.worker.register.timeout` (180s by default). A StatefulSet can
therefore proceed to the next worker even though this worker is absent from the
Master registration state. Please also add a test that exercises this
production path instead of only setting the flag directly.
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -935,6 +935,19 @@ private[celeborn] class Worker(
sb.toString()
}
+ override def healthCheck(): HandleResponse = {
+ val state = workerStatusManager.currentWorkerStatus.getState
Review Comment:
`currentWorkerStatus` is a plain mutable `var`. It is replaced inside
synchronized `transitionState`, but this HTTP read does not acquire the same
monitor and the field is not volatile or atomic. There is no happens-before
relationship, so the probe can observe stale `Normal` after the worker enters
`Idle`, decommission, or exit and return 200 instead of the documented 503.
Could we safely publish/read this state (for example, with `@volatile`, an
atomic reference, or a synchronized accessor) and add a non-`Normal` state 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]
