[Fix] Fix spark/flink starter script error on windows #6435

GangLiCN · 2024-03-02T10:05:31Z

This PR is to fix seatunnel issue 6386

Does this PR introduce any user-facing change?

No

How was this patch tested?

This pach can be tested via general windows specific flink/spark starter script.
Flink: start-seatunnel-flink-13(15)-connector-v2.cmd
Spark: start-seatunnel-spark-2(3)-connector-v2.cmd

For conf file, just using the example conf file from "seatunnel_home/config" dir.
v2.batch.config.template, v2.streaming.template

Check list

The purpose of this commit is to fix spark & flink job submit issue on windows platform.

Add more comments after last change..

zhilinli123 · 2024-03-05T07:22:34Z

...seatunnel-core-starter/src/main/java/org/apache/seatunnel/core/starter/utils/SystemUtil.java

+           os_type="Windows";
+        } else if (SystemUtils.IS_OS_MAC) {
+           os_type="Mac";
+        } else if (SystemUtils.IS_OS_LINUX) {


Do you mean change the code to the code style "switch ... case" ? If this is true, I'll modify as you suggested.

I don't think "switch... case" is needed here, because there are many different properties from SystemUtils(e.g. IS_OS_LINUX, IS_OS_WINDOWS,,,etc), if SystemUtils can return unique property "OS_TYPE" then we can change to Switch... case style easily.

zhilinli123 · 2024-03-05T07:22:55Z

...flink-starter-common/src/main/java/org/apache/seatunnel/core/starter/flink/FlinkStarter.java

+        if (local_os_type.toLowerCase().equals("windows")) {
+            cmd_flink="%FLINK_HOME%/bin/flink.cmd";
+        } else if (local_os_type.toLowerCase().equals("linux")) {             
+            cmd_flink="${FLINK_HOME}/bin/flink";
+        } else if (local_os_type.toLowerCase().equals("unknown")) {             
+          cmd_flink="error";
+        }


zhilinli123 · 2024-03-05T07:23:09Z

...spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/SparkStarter.java

+        if (local_os_type.toLowerCase().equals("windows")) {
+            cmd_spark="%SPARK_HOME%/bin/spark-submit.cmd";
+        } else if (local_os_type.toLowerCase().equals("linux")) {             
+            cmd_spark="${SPARK_HOME}/bin/spark-submit";
+        } else if (local_os_type.toLowerCase().equals("unknown")) {             
+          cmd_spark="error";
+        }


…anch

mege Issue6386 fix with "dev' branch

Hisoka-X

Please add test in e2e.

GangLiCN · 2024-03-07T02:47:57Z

Please add test in e2e.

Hi, please tell me the full detailed steps for "add test in e2e" since I'm fresh on this.

Bases on my understanding, it seems that it's unnecessary to add new test cases because current test case is enoguh to cover the bug fix. As I mentioned before, we can verify the fix like below:
"
The fix can be tested via general windows specific flink/spark starter script.
Flink: start-seatunnel-flink-13(15)-connector-v2.cmd
Spark: start-seatunnel-spark-2(3)-connector-v2.cmd

For conf file, just using the example conf file from "seatunnel_home/config" dir.
v2.batch.config.template, v2.streaming.template
"

Hisoka-X · 2024-03-07T02:59:15Z

Please add test in e2e.

Hi, please tell me the full detailed steps for "add test in e2e" since I'm fresh on this.

Bases on my understanding, it seems that it's unnecessary to add new test cases because current test case is enoguh to cover the bug fix. As I mentioned before, we can verify the fix like below: " The fix can be tested via general windows specific flink/spark starter script. Flink: start-seatunnel-flink-13(15)-connector-v2.cmd Spark: start-seatunnel-spark-2(3)-connector-v2.cmd

For conf file, just using the example conf file from "seatunnel_home/config" dir. v2.batch.config.template, v2.streaming.template "

Oh I see. How about add some UT to verify whether the output command is right. Our UT will exeucted both on windows and linux.

GangLiCN · 2024-03-07T03:21:32Z

Please add test in e2e.

Hi, please tell me the full detailed steps for "add test in e2e" since I'm fresh on this.
Bases on my understanding, it seems that it's unnecessary to add new test cases because current test case is enoguh to cover the bug fix. As I mentioned before, we can verify the fix like below: " The fix can be tested via general windows specific flink/spark starter script. Flink: start-seatunnel-flink-13(15)-connector-v2.cmd Spark: start-seatunnel-spark-2(3)-connector-v2.cmd
For conf file, just using the example conf file from "seatunnel_home/config" dir. v2.batch.config.template, v2.streaming.template "

Oh I see. How about add some UT to verify whether the output command is right. Our UT will exeucted both on windows and linux.

What does UT mean ? user testcase ?

Hisoka-X · 2024-03-07T03:27:29Z

Please add test in e2e.

Hi, please tell me the full detailed steps for "add test in e2e" since I'm fresh on this.
Bases on my understanding, it seems that it's unnecessary to add new test cases because current test case is enoguh to cover the bug fix. As I mentioned before, we can verify the fix like below: " The fix can be tested via general windows specific flink/spark starter script. Flink: start-seatunnel-flink-13(15)-connector-v2.cmd Spark: start-seatunnel-spark-2(3)-connector-v2.cmd
For conf file, just using the example conf file from "seatunnel_home/config" dir. v2.batch.config.template, v2.streaming.template "

Oh I see. How about add some UT to verify whether the output command is right. Our UT will exeucted both on windows and linux.

What does UT mean ? user testcase ?

unit test. you can refer

seatunnel/seatunnel-core/seatunnel-flink-starter/seatunnel-flink-starter-common/src/test/java/org/apache/seatunnel/core/starter/flink/TestFlinkParameter.java

Line 37 in ec533ec

public class TestFlinkParameter {

so on

GangLiCN · 2024-03-07T07:36:38Z

Please add test in e2e.

Hi, please tell me the full detailed steps for "add test in e2e" since I'm fresh on this.
Bases on my understanding, it seems that it's unnecessary to add new test cases because current test case is enoguh to cover the bug fix. As I mentioned before, we can verify the fix like below: " The fix can be tested via general windows specific flink/spark starter script. Flink: start-seatunnel-flink-13(15)-connector-v2.cmd Spark: start-seatunnel-spark-2(3)-connector-v2.cmd
For conf file, just using the example conf file from "seatunnel_home/config" dir. v2.batch.config.template, v2.streaming.template "

Oh I see. How about add some UT to verify whether the output command is right. Our UT will exeucted both on windows and linux.

What does UT mean ? user testcase ?

unit test. you can refer

seatunnel/seatunnel-core/seatunnel-flink-starter/seatunnel-flink-starter-common/src/test/java/org/apache/seatunnel/core/starter/flink/TestFlinkParameter.java

Line 37 in ec533ec

public class TestFlinkParameter {

so on

I checked the source code of "TestFlinkParameter.java" and feel a bit confused.
It imports two new class:
import org.apache.seatunnel.core.starter.flink.args.FlinkCommandArgs;
import org.apache.seatunnel.core.starter.flink.utils.EnvironmentUtil;

I can't find these classes in dev branch of base repo(apache/seatunnel)'s code line,
Has these two java classes added recently ?

About this PR, Is it also necessary to introduce some new java classes to finish a simple unit test case ?
Do we have a guideline or standard rule to submit unit test case ?

Hisoka-X · 2024-03-07T07:41:57Z

I can't find these classes in dev branch of base repo(apache/seatunnel)'s code line,

You can search it

About this PR, Is it also necessary to introduce some new java classes to finish a simple unit test case ?

Yes, just create new java file with new test case. The TestFlinkParameter.java just a demo of unit test in seatunnel

hailin0 · 2024-03-11T04:32:12Z

please merge dev branch update
6442

The purpose of this commit is to fix spark & flink job submit issue on windows platform.

Add more comments after last change..

…anch

…into pr/6435

hailin0 · 2024-03-11T05:46:54Z

Sorry, I made an error operation and I'm trying to recover it

…in-port into pr/6435"

GangLiCN · 2024-03-11T13:31:44Z

I've finished java unit test cases and tested successfully.

BTW, I also fixed windows specific starter script errors(e.g. start-seatunnel-flink-15-connector-v2.cmd, start-seatunnel-spark-3-connector-v2.cmd) because in 2.3.4 release version running these scripts will trigger "JAVA CLASSNOTFOUNDEXCEPTION".

seatunnel_issue6386-fix- with_java_unit_test

hailin0 · 2024-03-12T03:08:58Z

pom.xml

@@ -55,7 +55,7 @@

    <properties>
        <!--todo The classification is too confusing, reclassify by type-->
-        <revision>2.3.5-SNAPSHOT</revision>
+        <revision>2.3.4</revision>


Ok, I forgot to modify this file because on my local windows environment setting "2.3.5-SNAPSHOT" will cause build fails.

fix incorrect version issue from [pom.xml]

hailin0 · 2024-03-12T13:28:52Z

please merge dev branch update 6442

@GangLiCN
please fix it

hailin0 · 2024-03-14T02:14:36Z

hailin0 · 2024-03-14T02:15:59Z

If the merge conflict cannot be resolved, you can resubmit the PR

GangLiCN · 2024-03-15T03:20:35Z

I closed this PR and submitted a new PR：
#6511

GangLiCN added 4 commits February 1, 2024 12:42

New code commit

8e08b31

The purpose of this commit is to fix spark & flink job submit issue on windows platform.

Add more comments

5ac6f7e

Add more comments after last change..

fix issue6386

7334d51

Merge branch 'apache:dev' into dev

8216b25

zhilinli123 reviewed Mar 5, 2024

View reviewed changes

Hisoka-X linked an issue Mar 5, 2024 that may be closed by this pull request

[Bug] [SeaTunnel-flink/spark-starter-common] The full command to submit flink/spark job is not cross-platform compatible #6386

Open

3 tasks

Hisoka-X added the bug label Mar 5, 2024

Hisoka-X changed the title ~~[Bugfix][seatunnel--flink/spark-starter-common]~~ [Fix] Fix spark/flink starter script error on windows Mar 5, 2024

GangLiCN added 3 commits March 6, 2024 15:12

fix issue6386 bases on review comments

2c084e3

fix issue6386 again: resync SparkStarter.java from base repo's dev br…

fb1dead

…anch

Merge pull request #1 from GangLiCN/issue6386-fix

283f37d

mege Issue6386 fix with "dev' branch

Hisoka-X mentioned this pull request Mar 6, 2024

[Bugfix][seatunnel--flink/spark-starter-common] windows specific issue fix #6436

Closed

Hisoka-X reviewed Mar 6, 2024

View reviewed changes

Hisoka-X added the First-time contributor First-time contributor label Mar 6, 2024

GangLiCN and others added 6 commits March 11, 2024 13:30

New code commit

4a76471

The purpose of this commit is to fix spark & flink job submit issue on windows platform.

Add more comments

394f2fd

Add more comments after last change..

fix issue6386

951028a

fix issue6386 bases on review comments

655ae6b

fix issue6386 again: resync SparkStarter.java from base repo's dev br…

c5fd966

…anch

Merge branch 'dev' of https://github.com/GangLiCN/seatunnel-win-port …

9e1a42c

…into pr/6435

hailin0 and others added 3 commits March 11, 2024 14:38

Revert "Merge branch 'dev' of https://github.com/GangLiCN/seatunnel-w…

7bad23d

…in-port into pr/6435"

fix issue6386 and windows specific launch cmd script error

ca4c88f

Merge branch 'dev' into issue6386_fix_with_unit_test

37994c9

Merge pull request #2 from GangLiCN/issue6386_fix_with_unit_test

b1d904c

seatunnel_issue6386-fix- with_java_unit_test

hailin0 reviewed Mar 12, 2024

View reviewed changes

GangLiCN added 2 commits March 12, 2024 13:51

fix incorrect version issue from [pom.xml]

88eaa44

Merge pull request #3 from GangLiCN/fix_pom_xml_version_issue

6b5429e

fix incorrect version issue from [pom.xml]

GangLiCN added 2 commits March 13, 2024 11:36

Merge remote-tracking branch 'upstream/dev' into dev

d1199de

merge dev branch doc fix

f364600

GangLiCN closed this by deleting the head repository Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Fix spark/flink starter script error on windows #6435

[Fix] Fix spark/flink starter script error on windows #6435

GangLiCN commented Mar 2, 2024

zhilinli123 Mar 5, 2024

GangLiCN Mar 6, 2024

GangLiCN Mar 6, 2024

zhilinli123 Mar 5, 2024

GangLiCN Mar 6, 2024

zhilinli123 Mar 5, 2024

GangLiCN Mar 7, 2024

Hisoka-X left a comment •

edited

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

hailin0 commented Mar 11, 2024

hailin0 commented Mar 11, 2024

GangLiCN commented Mar 11, 2024

hailin0 Mar 12, 2024

GangLiCN Mar 12, 2024

GangLiCN Mar 12, 2024

hailin0 commented Mar 12, 2024 •

edited

hailin0 commented Mar 14, 2024

hailin0 commented Mar 14, 2024

GangLiCN commented Mar 15, 2024

[Fix] Fix spark/flink starter script error on windows #6435

[Fix] Fix spark/flink starter script error on windows #6435

Conversation

GangLiCN commented Mar 2, 2024

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hisoka-X left a comment • edited

Choose a reason for hiding this comment

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

GangLiCN commented Mar 7, 2024

Hisoka-X commented Mar 7, 2024

hailin0 commented Mar 11, 2024

hailin0 commented Mar 11, 2024

GangLiCN commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hailin0 commented Mar 12, 2024 • edited

hailin0 commented Mar 14, 2024

hailin0 commented Mar 14, 2024

GangLiCN commented Mar 15, 2024

Hisoka-X left a comment •

edited

hailin0 commented Mar 12, 2024 •

edited