Repository navigation
Conversation
Introduce the worker-side SPI that lets an external signal ask a worker to decommission itself, the default no-op checker, a built-in checker that signals once a marker file exists, and the three celeborn.worker.decommission.signal.* configs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
Move the delayed System.exit thread into an overridable scheduleSystemExit() and expose inExitStatus() within the worker package, so tests can drive real Decommission transitions without exiting the JVM. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
The monitor loads the configured checker (failing fast with the config key and class name on any error), polls it on a dedicated daemon thread, skips polling while the worker is already exiting, treats checker exceptions as no signal, and triggers decommission at most once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
The worker creates the configured checker before allocating any other
resource, starts polling once initialised, stops the monitor on stop, and
triggers decommission through the same exit("DECOMMISSION") path as the
REST API.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
…r init Extract the exception-swallowing checker poll into pollChecker() to flatten checkOnce(), and use getOrElse(throw ...) for the required signal file path. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
Treat LinkageError from a checker as recoverable so polling keeps running and a failing checker class still fails startup with the config key. Log at ERROR if a fatal error stops polling, if the trigger itself fails (not retried), and skip triggering when the monitor is stopped while a poll is in flight. Add a Worker test seam for WorkerStatusManager and a WorkerSuite test that runs the production DECOMMISSION exit path from the monitor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
…utdown Use this.conf in the WorkerSuite anonymous Worker subclass so the test compiles under Scala 2.13 with -Xfatal-warnings. Handle interruption of an in-flight poll during stop() quietly instead of logging a fatal error, stop the unstarted monitor in the trigger-failure test, and document why the DECOMMISSION test leaves the shutdown hook timeout updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
venkata91
changed the title
Rename the configs to celeborn.worker.decommission.signal.checker and celeborn.worker.decommission.signal.checkInterval. Instantiate checkers through Utils.instantiateClassWithCelebornConf with a CelebornConf constructor and drop init(). Create the monitor at the start of initialize() so a failing constructor never leaks the checker. Keep the trigger when getReason throws a LinkageError, and wait briefly for an in-flight check before closing the checker. Replace the overridable test seams with an injectable exit action on WorkerStatusManager. Fix the Kubernetes preStop example in the docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1816769b-cdd1-4c85-be0b-d11a935c03d8
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Adds a pluggable checker that lets a worker decommission itself when something outside Celeborn says the node is going away.
DecommissionSignalCheckerinterface in the worker module. Implementations take aCelebornConfconstructor. The worker loads the class set inceleborn.worker.decommission.signal.checkerand fails at startup if it can't.celeborn.worker.decommission.signal.checkInterval(30s by default). When it returns true, the worker calls the existingexit("DECOMMISSION"), same as the REST API. It fires at most once, and checker errors are logged and treated as a no.FileDecommissionSignalCheckeris included as a simple option: it decommissions the worker once a configured file exists.docs/decommissioning.md, anddocs/configuration/worker.mdis regenerated.Why are the changes needed?
https://issues.apache.org/jira/browse/CELEBORN-2536
Today decommission has to come from outside, through the worker or master REST API. But in cloud and autoscaled setups, the first heads-up usually reaches the node itself, like a spot preemption notice or a node drain. So every deployment ends up writing a sidecar that watches for it and calls the API. This lets the worker act on it directly.
Related: #3698 (decommission on SIGTERM). I think the two are complementary. That one handles the worker already being killed, this one acts before that.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
Three new worker configs (
celeborn.worker.decommission.signal.checker,.checkInterval,.file.path) and the publicDecommissionSignalCheckerinterface. Nothing changes unless a checker is configured.How was this patch tested?
New
DecommissionSignalMonitorSuiteandFileDecommissionSignalCheckerSuite, plusWorkerSuitecases for loading the checker, failing on a bad class, and reachingInDecommissionthrough the real exit path.Ran locally on JDK 17, all passed:
build/mvn -pl common,worker spotless:checkbuild/mvn clean test -pl common -am -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuitebuild/mvn test -pl worker -am '-Dtest=org.apache.celeborn.service.deploy.worker.**' -DwildcardSuites=org.apache.celeborn.service.deploy.worker(Scala 2.12)build/mvn test -Pspark-4.0 -pl worker -am -Dtest=none -DwildcardSuites=org.apache.celeborn.service.deploy.worker.decommission,org.apache.celeborn.service.deploy.worker.WorkerStatusManagerSuite,org.apache.celeborn.service.deploy.worker.WorkerSuite(Scala 2.13)build/mvn apache-rat:check -pl common,workerOn my arm64 Mac I also had to pass
-Dgrpc.version=1.44.0-local-arm64-stub, which isn't part of this change.Drafted with help from GitHub Copilot CLI.