strelok89 commented on code in PR #3832:
URL: https://github.com/apache/celeborn/pull/3832#discussion_r3897472595
##########
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:
Fixed, though not by clearing `registered` — that flag also gates RPC
serving. `FetchHandler#checkRegistered` and `PushDataHandler#checkRegistered`
read it, and `TransportRequestHandler#checkRegistered`
(`TransportRequestHandler.java:169`) fails every push RPC and chunk fetch with
"Worker Not Registered!" while it is false. Clearing it would reject live
traffic for the whole re-registration window, even though the worker still
holds the data that clients have locations for.
Instead I added a separate `registeredInMasterView` flag, cleared as soon as
a heartbeat response reports `registered == false` and restored on successful
registration. Only `/health` reads it; `registered` keeps its current semantics.
To make the production path testable I extracted
`Worker#handleHeartbeatResponse`, and the new test in `WorkerSuite` records the
flag at the moment the re-registration RPC is issued, so it asserts the
ordering rather than just the end state.
If the project would rather fence traffic when the master loses a worker,
clearing `registered` is a defensible alternative — happy to switch, but that
seems like a separate discussion from the health endpoint.
##########
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:
Good catch — `currentWorkerStatus` is now `@volatile`, and I added a test
that transitions the worker to `InDecommission` and asserts `/health` returns
503.
--
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]
