[cxzl25]

What changes were proposed in this pull request?

Allow overriding celeborn.worker.decommission.forceExitTimeout per decommission via an optional timeout parameter on the worker exit API — root /exit, v1 /api/v1/workers/exit, and the CLI --exit-timeout option — without restarting the worker or editing configuration.

Why are the changes needed?

celeborn.worker.decommission.forceExitTimeout (default 6h) is not dynamically changeable: it is read once at decommission start and baked into both the ShutdownHookManager hook ceiling and the decommissionWorker drain loop. There is currently no way to shorten it for a single decommission other than editing the config + restarting, and
once a decommission is running the value is fixed.

Does this PR resolve a correctness bug?

• Yes

Does this PR introduce any user-facing change?

• Yes

How was this patch tested?

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds support for overriding celeborn.worker.decommission.forceExitTimeout per decommission via an optional timeout parameter on worker exit APIs and CLI, avoiding worker restart/config edits.

Changes:

• Extend worker exit APIs (root /exit and v1 /api/v1/workers/exit) to accept an optional timeout duration.
• Update worker shutdown/decommission logic to use the effective timeout derived from API input or config.
• Regenerate/update OpenAPI client model and update docs/CLI help to document the new option.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/http/api/v1/WorkerResource.scala	Passes timeout from v1 JSON exit request to service layer and updates endpoint description.
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/http/api/ApiWorkerResource.scala	Adds timeout form param to legacy /exit endpoint and forwards to service layer.
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala	Parses timeout, updates shutdown hook timeout, and uses an override in decommission logic.
service/src/main/scala/org/apache/celeborn/server/common/HttpService.scala	Introduces exit(exitType, timeout) overload for service implementations.
openapi/openapi-client/src/main/openapi3/worker_rest_v1.yaml	Adds timeout field to WorkerExitRequest schema.
openapi/openapi-client/src/main/java/org/apache/celeborn/rest/v1/model/WorkerExitRequest.java	Adds generated timeout property and updates equality/hash/toString.
docs/restapi.md	Documents timeout in /exit API usage.
docs/decommissioning.md	Documents that forceExitTimeout is read once and can be overridden per decommission via API/CLI.
docs/celeborn_cli.md	Documents --exit-timeout option.
cli/src/main/scala/org/apache/celeborn/cli/worker/WorkerSubcommandImpl.scala	Adds timeout to exit request when provided and improves logging.
cli/src/main/scala/org/apache/celeborn/cli/worker/WorkerOptions.scala	Adds --exit-timeout CLI option.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

[yew1eb]

LGTM.

[Kalvin2077]

Tests needed at least:

• Using --exit DECOMMISSION together with --exit-timeout 30m.
• Passing timeout through both REST endpoints.
• Falling back to the configured timeout when timeout is omitted or invalid.

[Kalvin2077]

Keep the timeout option out of the mutually exclusive action group

WorkerOptions is installed by WorkerSubcommand as @ArgGroup(exclusive = true, multiplicity = "1"). Adding --exit-timeout here makes it mutually exclusive with --exit. Using the actual options class and the same group declaration with picocli 4.7.6, --exit DECOMMISSION --exit-timeout 30m returns code 2 with “--exit=exit_type, --exit-timeout=duration are mutually exclusive”, before the command runs. Consequently, the advertised CLI override cannot send a request. Move the modifier outside the action-selection group, or introduce a non-exclusive exit subgroup while retaining mutual exclusion between actions. Add a parsing regression test for the documented combination.

[github-actions]

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[zaynt4606]

Sorry for the delay in revisiting this PR, @cxzl25, and thanks for your patience. I checked the latest revision and still see two issues that need changes:

1. --exit-timeout is defined in WorkerOptions, which WorkerSubcommand installs as an exclusive @ArgGroup. As a result, the documented --exit DECOMMISSION --exit-timeout 30m combination fails argument parsing before any request is sent. Please move the timeout modifier outside the exclusive action group (or restructure the groups) and add a parsing test for this command. This was also noted in the earlier review thread.

2. Worker.exit updates decommissionTimeoutOverrideMs and the shutdown-hook timeout before calling doTransition. A second DECOMMISSION request can therefore change the first request's timeout even though WorkerStatusManager ignores the second transition once the worker is exiting. For example, 30m followed by a request without timeout before shutdown starts restores the configured timeout (6h by default); a shorter second timeout could also end the drain early. Please bind the timeout to the accepted transition and add a regression test with consecutive exit requests. This was also raised in the earlier review thread.

Thanks for addressing these.

[zaynt4606]

@cxzl25 I pushed 56b10da to address the remaining timeout issue. The timeout is now applied under the status transition lock only when a decommission request is accepted, so ignored duplicate or concurrent requests cannot overwrite it. The drain loop and shutdown hook use the same accepted timeout.

I also moved status manager initialization before the HTTP server and heartbeats start, so it is ready when exit requests arrive.

Could you take a look and let me know whether this approach works for you?