[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #4937: [Feature][Flink] Support parallelism input

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

chengshiwen commented on a change in pull request #4937:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/4937#discussion_r586380763



##########
File path: dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/Constants.java
##########
@@ -852,8 +854,9 @@ private Constants() {
 
     public static final String FLINK_JOB_MANAGE_MEM = "-yjm";
     public static final String FLINK_TASK_MANAGE_MEM = "-ytm";
-    public static final String FLINK_DETACH = "-d";

Review comment:
       -  `-d,--detached`, `-s,--fromSavepoint`, `--allowNonRestoredState` are optional and not common options, so they are more suitable to be placed in the `option parameters`.
   - `-p,--parallelism` is a common and frequently used option, so I add it to the code.
   - `-d,--detached` is not a good option for DolphinScheduler, which means `runs the job in detached mode`. This mode will cause that the task state and the yarn job state are different. We will see that the DolphinScheduler task ends early, but in fact the task on yarn is still running.
   - ` -sae,--shutdownOnAttachedExit` is better, which means `If the job is submitted in attached mode, perform a best-effort cluster shutdown when the CLI is terminated abruptly`
   - The proposals of the author of that pr are not all good, just both `Generic CLI` mode and  `-p,--parallelism` are good. This feature `-p, --parallelism` is what I wanted to add two months ago




----------------------------------------------------------------
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.

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