ruanwenjun commented on PR #17320:
URL:
https://github.com/apache/dolphinscheduler/pull/17320#issuecomment-3068587224
> ## Pull Request Overview
> This PR adds a configurable timeout for waiting on shell process
termination and enhances the kill logic to poll process status after sending
kill signals. It also updates various resource configurations and bumps the
Kubernetes Helm chart version.
>
> * Introduce `shell.kill.wait.timeout` property in common configs and add
`SHELL_KILL_WAIT_TIMEOUT` constant
> * Refactor `ProcessUtils.kill` to parse PIDs into a list, send signals,
and poll until processes exit
> * Update E2E/test properties, document new config in Helm values and
README, and bump chart version
>
> ### Reviewed Changes
> Copilot reviewed 10 out of 10 changed files in this pull request and
generated no comments.
>
> Show a summary per file
> File Description
> dolphinscheduler-task-plugin/.../common.properties Add
`shell.kill.wait.timeout` property
> ProcessUtils.java Parse PIDs into `List<Integer>`, implement polling with
timeout, add `isProcessAlive`
> dolphinscheduler-e2e/.../common.properties Add timeout property and fix
`appId.collect` syntax
> dolphinscheduler-common/.../common.properties Add
`shell.kill.wait.timeout` to test and main resources
> Constants.java Add `SHELL_KILL_WAIT_TIMEOUT` constant
> values.yaml Add `shell.kill.wait.timeout` under `conf`
> README.md Document `shell.kill.wait.timeout` in chart config
> Chart.yaml Bump `version` and `appVersion` to 3.1.1
> Comments suppressed due to low confidence (4)
>
**dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/constants/Constants.java:75**
>
> * [nitpick] The Javadoc for this constant is minimal. Consider expanding
it to explain that this property defines the wait timeout in seconds before
using SIGKILL.
>
> ```
> public static final String SHELL_KILL_WAIT_TIMEOUT =
"shell.kill.wait.timeout";
> ```
>
>
**dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java:54**
>
> * It looks like `List` and `ArrayList` are used later in this class but
not imported here. Please add `import java.util.List;` and `import
java.util.ArrayList;` to avoid compile errors.
>
> ```
> import java.util.stream.Collectors;
> ```
>
>
**dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java:168**
>
> * [nitpick] Mutating the original `pidList` inside this method can lead to
side effects if the list is reused elsewhere. Consider making a defensive copy
of the list before polling/removal.
>
> ```
> private static boolean sendKillSignal(String signal, List<Integer>
pidList, String tenantCode) {
> ```
>
>
**dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java:188**
>
> * [nitpick] The multiplication here assumes `SLEEP_TIME_MILLIS` maps
directly to milliseconds. It might be clearer to calculate `timeoutMillis =
TimeUnit.SECONDS.toMillis(SHELL_KILL_WAIT_TIMEOUT)` or document the
relationship explicitly.
>
> ```
> long timeoutMillis = SLEEP_TIME_MILLIS *
SHELL_KILL_WAIT_TIMEOUT;
> ```
Please take care about the comment from copilot.
--
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]
