[Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options #3771

MonsterChenzhuo · 2022-12-19T13:24:33Z

Purpose of this pull request

Check list

Code changed are covered with tests, or it does not need tests for reason:
If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
If you are contributing the connector code, please check that the following files are updated:
1. Update change log that in connector document. For more details you can refer to connector-v2
2. Update plugin-mapping.properties and add new connector information in it
3. Update the pom file of seatunnel-dist

…gine JVM Options

MonsterChenzhuo · 2022-12-19T13:47:37Z

hi @EricJoy2048,Considering the future jdk upgrade 11 or higher, the current JVM configuration items to have the ability to backward compatibility, I referred to some relevant information, will be designed into this format: 8-13:-XX:+UseConcMarkSweepGC on behalf of the current machine jdk running version between 8-13 this parameter will take effect, so I did not use . properties file format for implementation.

...-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JvmOptionsParser.java

Hisoka-X · 2022-12-28T04:00:45Z

Hi, can you add some test case for this? Thanks.

MonsterChenzhuo · 2022-12-29T10:01:38Z

@Hisoka-X Thanks for your review,I have made the changes.

Hisoka-X · 2023-01-06T02:08:25Z

Hi, e2e same like have some problem.

EricJoy2048 · 2023-02-08T11:11:29Z

Can you and some test for JDK, openjdk ?

EricJoy2048 · 2023-02-08T10:55:46Z

...unnel-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JavaVersion.java

+
+    static List<Integer> parse(final String value) {
+        if (!value.matches("^0*[0-9]+(\\.[0-9]+)*$")) {
+            throw new IllegalArgumentException(value);


Use a custom Exception is better.

EricJoy2048 · 2023-02-08T11:09:40Z

...-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JvmOptionsParser.java

+            parse(
+                    JavaVersion.majorVersion(JavaVersion.CURRENT),
+                    br,
+                    new JvmOptionConsumer() {


jvmOption -> jvmOptions.add(jvmOption) is better.

EricJoy2048 · 2023-02-08T11:10:22Z

...-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JvmOptionsParser.java

+                            jvmOptions.add(jvmOption);
+                        }
+                    },
+                    new InvalidLineConsumer() {


(lineNumber, line) -> invalidLines.put(lineNumber, line) is bettter.

MonsterChenzhuo · 2023-02-09T08:44:33Z

@EricJoy2048 Thanks for your review,I have made the changes.

seatunnel-common/src/main/java/org/apache/seatunnel/common/exception/CommonErrorCode.java

TyrantLucifer · 2023-02-09T15:18:33Z

...-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JvmOptionsParser.java

+            final int javaMajorVersion,
+            final BufferedReader br,
+            final JvmOptionConsumer jvmOptionConsumer,
+            final InvalidLineConsumer invalidLineConsumer){


Add a blank between ) and {

TyrantLucifer · 2023-02-09T15:24:55Z

seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-cluster.sh

 CLASS_PATH=${APP_DIR}/lib/*:${APP_JAR}

+ST_TMPDIR=`java -cp ${CLASS_PATH} org.apache.seatunnel.core.starter.seatunnel.jvm.TempDirectory`


Do We need to write a class to generate a temporary directory? Why not using shell instead of this? cc @EricJoy2048 @Hisoka-X

I'm not sure the temp dir use for? Can you give me some tips?

@TyrantLucifer Is the latter engine considered for deployment in a windows environment?
If the future expectation is compatible with windows need this class, so that the logic of .sh and .bat files will be the same

@MonsterChenzhuo Can you provide JAVA_OPTS after add ST_TMPDIR? Thanks

cc @EricJoy2048

MonsterChenzhuo · 2023-02-10T01:37:17Z

@TyrantLucifer Thanks for your review,I have made the changes.

Hisoka-X · 2023-02-10T06:19:07Z

...nel-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/TempDirectory.java

+            path = Paths.get(System.getProperty("java.io.tmpdir"), "seatunnel");
+            Files.createDirectories(path);
+        } else {
+            path = Files.createTempDirectory("seatunnel-");


Suggested change

path = Files.createTempDirectory("seatunnel-");

path = Files.createTempDirectory("seatunnel");

The purpose of - is to make it more aesthetically pleasing, followed by a string of random numbers to distinguish between multiple task instance directories

TyrantLucifer

LGTM

MonsterChenzhuo added 3 commits December 19, 2022 21:24

[Feature][SeaTunnel Engine] Add a jvm file to define the SeaTunnel En…

78de03e

…gine JVM Options

fix

5c2a6b9

fix

3494873

MonsterChenzhuo added 4 commits December 19, 2022 22:16

fix

975ceb7

fix

8080d8e

fix

69c30b8

fix

bd14550

Hisoka-X added feature New feature Zeta labels Dec 28, 2022

Hisoka-X reviewed Dec 28, 2022

View reviewed changes

...-starter/src/main/java/org/apache/seatunnel/core/starter/seatunnel/jvm/JvmOptionsParser.java Outdated Show resolved Hide resolved

MonsterChenzhuo added 8 commits December 29, 2022 13:28

Merge branch 'dev' into jvm

b57f51a

fix

b184965

fix

4c0c8da

fix

da22b71

fix

3fc2219

fix

1433110

fix

df0be70

fix

6e89257

Hisoka-X requested a review from EricJoy2048 December 29, 2022 10:06

MonsterChenzhuo added 3 commits December 29, 2022 18:47

fix

11edfa1

Merge branch 'dev' into jvm

fa92315

Merge branch 'dev' into jvm

9ce2e40

MonsterChenzhuo added 4 commits January 6, 2023 10:31

fix

56158e9

fix

9c655f4

fix

6d5d0ac

fix

1391385

MonsterChenzhuo and others added 7 commits January 7, 2023 00:08

Merge branch 'dev' into jvm

c8067e9

Merge branch 'dev' into jvm

2959332

fix

21eaa7c

fix

2ce90ae

Merge branch 'dev' into jvm

f4e3af9

fix

321a332

fix

8dee91c

EricJoy2048 reviewed Feb 8, 2023

View reviewed changes

MonsterChenzhuo added 2 commits February 8, 2023 21:42

fix

2900240

fix

fe39a5d

EricJoy2048 previously approved these changes Feb 9, 2023

View reviewed changes

github-actions bot added approved reviewed labels Feb 9, 2023

TyrantLucifer requested changes Feb 9, 2023

View reviewed changes

seatunnel-common/src/main/java/org/apache/seatunnel/common/exception/CommonErrorCode.java Show resolved Hide resolved

ashulin changed the title ~~[Feature][SeaTunnel Engine] Add a jvm file to define the SeaTunnel En…~~ [Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options Feb 9, 2023

github-actions bot removed approved reviewed labels Feb 9, 2023

TyrantLucifer reviewed Feb 9, 2023

View reviewed changes

fix

d53a302

MonsterChenzhuo dismissed EricJoy2048’s stale review via d53a302 February 10, 2023 01:36

Hisoka-X reviewed Feb 10, 2023

View reviewed changes

TyrantLucifer approved these changes Feb 10, 2023

View reviewed changes

Hisoka-X approved these changes Feb 10, 2023

View reviewed changes

github-actions bot added approved reviewed labels Feb 10, 2023

Hisoka-X merged commit f165b18 into apache:dev Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options #3771

[Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options #3771

MonsterChenzhuo commented Dec 19, 2022 •

edited

Loading

MonsterChenzhuo commented Dec 19, 2022

Hisoka-X commented Dec 28, 2022

MonsterChenzhuo commented Dec 29, 2022

Hisoka-X commented Jan 6, 2023

EricJoy2048 commented Feb 8, 2023

EricJoy2048 Feb 8, 2023

EricJoy2048 Feb 8, 2023

EricJoy2048 Feb 8, 2023

MonsterChenzhuo commented Feb 9, 2023

TyrantLucifer Feb 9, 2023

TyrantLucifer Feb 9, 2023

Hisoka-X Feb 10, 2023

MonsterChenzhuo Feb 10, 2023 •

edited

Loading

Hisoka-X Feb 10, 2023

TyrantLucifer Feb 10, 2023

MonsterChenzhuo commented Feb 10, 2023

Hisoka-X Feb 10, 2023

MonsterChenzhuo Feb 10, 2023

TyrantLucifer left a comment

		CLASS_PATH=${APP_DIR}/lib/*:${APP_JAR}

		ST_TMPDIR=`java -cp ${CLASS_PATH} org.apache.seatunnel.core.starter.seatunnel.jvm.TempDirectory`

	path = Files.createTempDirectory("seatunnel-");
	path = Files.createTempDirectory("seatunnel");

[Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options #3771

[Feature][Zeta] Add a jvm.properties file to define the SeaTunnel Zeta JVM Options #3771

Conversation

MonsterChenzhuo commented Dec 19, 2022 • edited Loading

Purpose of this pull request

Check list

MonsterChenzhuo commented Dec 19, 2022

Hisoka-X commented Dec 28, 2022

MonsterChenzhuo commented Dec 29, 2022

Hisoka-X commented Jan 6, 2023

EricJoy2048 commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsterChenzhuo commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsterChenzhuo Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsterChenzhuo commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TyrantLucifer left a comment

Choose a reason for hiding this comment

MonsterChenzhuo commented Dec 19, 2022 •

edited

Loading

MonsterChenzhuo Feb 10, 2023 •

edited

Loading