Conversation
There was a problem hiding this comment.
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
/exitand v1/api/v1/workers/exit) to accept an optionaltimeoutduration. - 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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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.
|
LGTM. |
Kalvin2077
left a comment
There was a problem hiding this comment.
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.
| private[worker] var exitType: String = _ | ||
|
|
||
| @Option( | ||
| names = Array("--exit-timeout"), |
There was a problem hiding this comment.
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.
What changes were proposed in this pull request?
Allow overriding
celeborn.worker.decommission.forceExitTimeoutper decommission via an optionaltimeoutparameter on the workerexitAPI — root/exit, v1/api/v1/workers/exit, and the CLI--exit-timeoutoption — without restarting the worker or editing configuration.Why are the changes needed?
celeborn.worker.decommission.forceExitTimeout(default6h) is not dynamically changeable: it is read once at decommission start and baked into both theShutdownHookManagerhook ceiling and thedecommissionWorkerdrain loop. There is currently no way to shorten it for a single decommission other than editing the config + restarting, andonce a decommission is running the value is fixed.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?