[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1636: [Feature][Flink] Rewrite start-seatunnel-flink script with starter

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

yx91490 commented on a change in pull request #1636:
URL: https://github.com/apache/incubator-seatunnel/pull/1636#discussion_r841137587



##########
File path: seatunnel-core/seatunnel-core-flink/src/main/bin/start-seatunnel-flink.sh
##########
@@ -16,87 +16,24 @@
 # limitations under the License.
 #
 
-# copy command line arguments
-
-function usage() {
-  echo "Usage: start-seatunnel-flink.sh [options]"
-  echo "  options:"
-  echo "    --config, -c FILE_PATH        Config file"
-  echo "    --variable, -i PROP=VALUE     Variable substitution, such as -i city=beijing, or -i date=20190318"
-  echo "    --check, -t                   Check config"
-  echo "    --help, -h                    Show this help message"
-}
-
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]] || [[ $# -le 1 ]]; then
-  usage
-  exit 0
-fi
-
-is_exist() {
-    if [ -z $1 ]; then
-      usage
-      exit 1
-    fi
-}
-
-PARAMS=""
-while (( "$#" )); do
-  case "$1" in
-    -c|--config)
-      CONFIG_FILE=$2
-      is_exist ${CONFIG_FILE}
-      shift 2
-      ;;
-
-    -i|--variable)
-      variable=$2
-      is_exist ${variable}
-      java_property_value="-D${variable}"
-      variables_substitution="${java_property_value} ${variables_substitution}"
-      shift 2
-      ;;
-
-    *) # preserve positional arguments
-      PARAMS="$PARAMS $1"
-      shift
-      ;;
-
-  esac
-done
-
-if [ -z ${CONFIG_FILE} ]; then
-  echo "Error: The following option is required: [-c | --config]"
-  usage
-  exit -1
-elif [ ! -f ${CONFIG_FILE} ];then
-  echo "Error: Config file ${CONFIG_FILE} does not exists! Please check it."
-  exit -1
-fi
-
-# set positional arguments in their proper place
-eval set -- "$PARAMS"
-
-BIN_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
-APP_DIR=$(dirname ${BIN_DIR})
+set -eu
+APP_DIR=$(cd $(dirname ${0})/../;pwd)
 CONF_DIR=${APP_DIR}/config
-PLUGINS_DIR=${APP_DIR}/lib
-DEFAULT_CONFIG=${CONF_DIR}/application.conf
-CONFIG_FILE=${CONFIG_FILE:-$DEFAULT_CONFIG}
-
-assemblyJarName=$(find ${PLUGINS_DIR} -name seatunnel-core-flink*.jar)
+APP_JAR=${APP_DIR}/lib/seatunnel-core-flink.jar
 
 if [ -f "${CONF_DIR}/seatunnel-env.sh" ]; then
-    source ${CONF_DIR}/seatunnel-env.sh
+    . "${CONF_DIR}/seatunnel-env.sh"
 fi
 
-string_trim() {
-    echo $1 | awk '{$1=$1;print}'
-}
-
-JVM_ARGS=$(string_trim "${variables_substitution}")

Review comment:
       you changed the JVM_ARGS usage method, I don't known whether will cause problem:(
   especially if variables contains quote, blank space, etc.

##########
File path: seatunnel-core/seatunnel-core-flink/src/main/java/org/apache/seatunnel/FlinkStarter.java
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.seatunnel;
+
+import org.apache.seatunnel.command.FlinkCommandArgs;
+import org.apache.seatunnel.common.config.Common;
+
+import com.beust.jcommander.JCommander;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * The SeaTunnel flink starter. This class is responsible for generate the final flink job execute command.
+ */
+public class FlinkStarter implements Starter {
+
+    private static final String APP_NAME = SeatunnelFlink.class.getName();
+    private static final int USAGE_EXIT_CODE = 234;
+    private static final String APP_JAR_NAME = "seatunnel-core-flink.jar";
+
+    /**
+     * Flink parameters, used by flink job itself. e.g. `-m yarn-cluster`
+     */
+    private final List<String> flinkParams = new ArrayList<>();
+
+    /**
+     * SeaTunnel parameters, used by SeaTunnel application. e.g. `-c config.conf`
+     */
+    private final FlinkCommandArgs flinkCommandArgs;
+
+    /**
+     * SeaTunnel flink job jar.
+     */
+    private final String appJar;
+
+    public FlinkStarter(String[] args) {

Review comment:
       can be private constructor

##########
File path: seatunnel-core/seatunnel-core-flink/src/main/java/org/apache/seatunnel/FlinkStarter.java
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.seatunnel;
+
+import org.apache.seatunnel.command.FlinkCommandArgs;
+import org.apache.seatunnel.common.config.Common;
+
+import com.beust.jcommander.JCommander;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * The SeaTunnel flink starter. This class is responsible for generate the final flink job execute command.
+ */
+public class FlinkStarter implements Starter {
+
+    private static final String APP_NAME = SeatunnelFlink.class.getName();
+    private static final int USAGE_EXIT_CODE = 234;
+    private static final String APP_JAR_NAME = "seatunnel-core-flink.jar";
+
+    /**
+     * Flink parameters, used by flink job itself. e.g. `-m yarn-cluster`
+     */
+    private final List<String> flinkParams = new ArrayList<>();
+
+    /**
+     * SeaTunnel parameters, used by SeaTunnel application. e.g. `-c config.conf`
+     */
+    private final FlinkCommandArgs flinkCommandArgs;
+
+    /**
+     * SeaTunnel flink job jar.
+     */
+    private final String appJar;
+
+    public FlinkStarter(String[] args) {
+        this.flinkCommandArgs = parseArgs(args);
+        // set the deployment mode, used to get the job jar path.
+        Common.setDeployMode(flinkCommandArgs.getDeployMode().getName());
+        this.appJar = Common.appLibDir().resolve(APP_JAR_NAME).toString();
+    }
+
+    @SuppressWarnings("checkstyle:RegexpSingleline")
+    public static void main(String[] args) {
+        FlinkStarter flinkStarter = new FlinkStarter(args);
+        List<String> command = flinkStarter.buildCommands();
+        String finalFLinkCommand = String.join(" ", command);
+        System.out.println(finalFLinkCommand);
+    }
+
+    /**
+     * Parse seatunnel args.
+     *
+     * @param args args
+     * @return FlinkCommandArgs
+     */
+    private FlinkCommandArgs parseArgs(String[] args) {
+        FlinkCommandArgs flinkCommandArgs = new FlinkCommandArgs();
+        JCommander jCommander = JCommander.newBuilder()
+            .programName("start-seatunnel-flink.sh")
+            .addObject(flinkCommandArgs)
+            .acceptUnknownOptions(true)
+            .args(args)
+            .build();
+        // The args is not belongs to seatunnel, add into flink params
+        flinkParams.addAll(jCommander.getUnknownOptions());
+        if (flinkCommandArgs.isHelp()) {
+            jCommander.usage();
+            System.exit(USAGE_EXIT_CODE);
+        }
+        return flinkCommandArgs;
+    }
+
+    @Override
+    public List<String> buildCommands() {
+        List<String> command = new ArrayList<>();
+        command.add("${FLINK_HOME}/bin/flink run");
+        command.addAll(flinkParams);
+        command.add("-c " + APP_NAME);
+        command.add(appJar);
+        command.add("--config " + flinkCommandArgs.getConfigFile());

Review comment:
       use '+' maybe make command not atomic, split to 2 add step may be better.




-- 
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@seatunnel.apache.org

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