[GitHub] [dolphinscheduler] SbloodyS commented on a diff in pull request #10217: [Feature] [MLOps] support mlflow deploy with docker compose

[GitBox <gi...@apache.org>]

SbloodyS commented on code in PR #10217:
URL: https://github.com/apache/dolphinscheduler/pull/10217#discussion_r882377386


##########
dolphinscheduler-task-plugin/dolphinscheduler-task-mlflow/src/main/java/org/apache/dolphinscheduler/plugin/task/mlflow/MlflowTask.java:
##########
@@ -98,10 +98,15 @@ public void handle() throws Exception {
     @Override
     public void cancelApplication(boolean cancelApplication) throws Exception {
         // cancel process
-        shellCommandExecutor.cancelApplication();
+        if (mlflowParameters.getDeployType().equals(MlflowConstants.MLFLOW_MODELS_DEPLOY_TYPE_DOCKER_COMPOSE)) {
+            String envCommand = mlflowParameters.getDockerComposeEnvCommand();
+            shellCommandExecutor.run(envCommand + "\n" + MlflowConstants.DOCKER_COMPOSE_CANCEL);

Review Comment:
   > we should better have a cancel operator here, but IMO, we can only cancel some tasks running for a long time , such as `sleep 200` in bash which will sleep 200s, and users want to stop them, they could call cancelApp, but you here start the task by fixed `docker compose` command and whether success or fail will exit the command quickly, Am I right? @caishunfeng
   
   Agreed. We should cancel it by using ```docker rm -f``` as discussed in https://github.com/apache/dolphinscheduler/pull/10217/files#r882267962.



-- 
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: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org