SbloodyS commented on code in PR #17320: URL: https://github.com/apache/dolphinscheduler/pull/17320#discussion_r2189534804
########## dolphinscheduler-common/src/main/resources/common.properties: ########## @@ -104,4 +107,4 @@ shell.env_source_list= shell.interceptor.type=bash # Whether to enable remote logging -remote.logging.enable=false +remote.logging.enable=false Review Comment: Please add blank lines at the end of each line. ########## dolphinscheduler-e2e/dolphinscheduler-e2e-case/src/test/resources/docker/file-manage/common.properties: ########## @@ -111,4 +114,4 @@ ml.mlflow.preset_repository=https://github.com/apache/dolphinscheduler-mlflow ml.mlflow.preset_repository_version="main" # way to collect applicationId: log(original regex match), aop -appId.collect: log +appId.collect=log Review Comment: Same here. ########## dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java: ########## @@ -244,11 +291,12 @@ public static void cancelApplication(TaskExecutionContext taskExecutionContext) taskExecutionContext.setAppIds(String.join(TaskConstants.COMMA, appIds)); } if (CollectionUtils.isEmpty(appIds)) { - log.info("The appId is empty"); + log.info("The appId is empty, so there is no need to kill yarn application."); Review Comment: ```suggestion log.info("The appId is empty"); ``` We don't need this since not all shell task are yarn type. ########## dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java: ########## @@ -144,18 +155,54 @@ public static boolean kill(@NonNull TaskExecutionContext request) { */ private static boolean sendKillSignal(String signal, String pids, String tenantCode) { try { + // 1. Send the kill signal String killCmd = String.format("kill -s %s %s", signal, pids); + log.info("Kill command: {}, trying to terminate process", killCmd); Review Comment: ```suggestion ``` We don't need this log since we already log it in L162. ########## dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java: ########## @@ -144,18 +155,54 @@ public static boolean kill(@NonNull TaskExecutionContext request) { */ private static boolean sendKillSignal(String signal, String pids, String tenantCode) { try { + // 1. Send the kill signal String killCmd = String.format("kill -s %s %s", signal, pids); + log.info("Kill command: {}, trying to terminate process", killCmd); killCmd = OSUtils.getSudoCmd(tenantCode, killCmd); log.info("Sending {} to process group: {}, command: {}", signal, pids, killCmd); OSUtils.exeCmd(killCmd); + // 2. Wait for the process to respond to the signal + ThreadUtils.sleep(SLEEP_TIME_MILLIS * PROCESS_STATUS_CHECK_DELAY); + + // 3. Check if the processes are still running + String[] pidArray = PID_PATTERN.split(pids); + for (String pid : pidArray) { + // Check if each PID is still alive + if (isProcessAlive(Integer.parseInt(pid))) { + log.info("Kill command: {}, kill failed, the process: {} is still running", killCmd, pid); + // Return false if any process is still alive + return false; + } + } + log.info("Kill command: {}, kill succeeded", killCmd); + // All processes have been successfully terminated return true; } catch (Exception e) { log.error("Error sending {} to process: {}", signal, pids, e); return false; } } + /** + * Check if a process with the specified PID is alive. + * + * @param pid the process ID to check + * @return true if the process exists and is running, false otherwise + */ + private static boolean isProcessAlive(int pid) { + try { + // Use kill -0 to check if the process exists; it does not actually send a signal + String checkCmd = String.format("kill -0 %d", pid); + OSUtils.exeCmd(checkCmd); Review Comment: Using `OSUtils.getSudoCmd` to get `killCmd` ########## dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/utils/ProcessUtils.java: ########## @@ -144,18 +155,54 @@ public static boolean kill(@NonNull TaskExecutionContext request) { */ private static boolean sendKillSignal(String signal, String pids, String tenantCode) { try { + // 1. Send the kill signal String killCmd = String.format("kill -s %s %s", signal, pids); + log.info("Kill command: {}, trying to terminate process", killCmd); killCmd = OSUtils.getSudoCmd(tenantCode, killCmd); log.info("Sending {} to process group: {}, command: {}", signal, pids, killCmd); OSUtils.exeCmd(killCmd); + // 2. Wait for the process to respond to the signal + ThreadUtils.sleep(SLEEP_TIME_MILLIS * PROCESS_STATUS_CHECK_DELAY); + + // 3. Check if the processes are still running + String[] pidArray = PID_PATTERN.split(pids); + for (String pid : pidArray) { + // Check if each PID is still alive + if (isProcessAlive(Integer.parseInt(pid))) { + log.info("Kill command: {}, kill failed, the process: {} is still running", killCmd, pid); + // Return false if any process is still alive + return false; + } + } + log.info("Kill command: {}, kill succeeded", killCmd); Review Comment: ```suggestion log.debug("Kill command: {}, kill succeeded", killCmd); ``` -- 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]
