You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@seatunnel.apache.org by GitBox <gi...@apache.org> on 2022/04/25 11:03:41 UTC
[GitHub] [incubator-seatunnel] legendtkl opened a new pull request, #1742: [Feature] [seatunnel-core-flink] flink job support application mode
legendtkl opened a new pull request, #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742
## Purpose of this pull request
Support Flink job run in application mode, ref: https://github.com/apache/incubator-seatunnel/issues/1741
## Check list
* [ yes] Code changed are covered with tests, or it does not need tests for reason:
* [ yes] If any new Jar binary package adding in your PR, please add License Notice according
[New License Guide](https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/contribution/new-license.md)
* [ yes] If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857536465
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
Hi, @ruanwenjun , thanks for you review. Let's have a more detailed discuss.
Flink use `run` and `run-application` to do different action.
* run
* yarn-session, kubernetes-session for `session` mode
* yarn-per-job for `per-job` mode
* run-application: for application mode
* kubernetes-application, yarn-application
The sub commands `yarn-session`, `kubernetes-session`, `yarn-per-job`, `kubernetes-application`, `yarn-application` could be config with `execution.target`.
Now I want to distinguish the action 'run' and 'run-application', so I add `--application-mode`.
In your point, should we use executeMode (value could be 'session', 'per-job', 'application') to do more commands generate with `run`/`run-application` and `execution.target` check? or just `run` and `run-application` differentiation?
Thanks.
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857672403
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"-r", "--run-mode"},
+ description = "job run mode, run or run-application")
+ private String runMode;
Review Comment:
@ruanwenjun I think optional will be a better user experience, as we already set the default value.
What do you think?
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#issuecomment-1108566980
Hi, @ruanwenjun , I have update the PR and doc. Would you help review it again?
ps: In flink, the execution mode is used to represent job execution (Streamng or Batch), so I use run-mode here to specify `run` or `run-application`.
Thanks,
Kelu.
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857643156
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"-r", "--run-mode"},
+ description = "job run mode, run or run-application")
+ private String runMode;
Review Comment:
```suggestion
@Parameter(names = {"-r", "--run-mode"},
description = "job run mode, run or run-application",
required = true)
private String runMode = "run";
```
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857536465
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
Hi, @ruanwenjun , thanks for you review. Let's have a more detail discuss.
Flink use `run` and `run-application` to do different action.
* run
* yarn-session, kubernetes-session for `session` mode
* yarn-per-job for `per-job` mode
* run-application: for application mode
* kubernetes-application, yarn-application
The sub commands `yarn-session`, `kubernetes-session`, `yarn-per-job`, `kubernetes-application`, `yarn-application` could be config with `execution.target`.
Now I want to distinguish the action 'run' and 'run-application', so I add `--application-mode`.
In your point, should we use executeMode (value could be 'session', 'per-job', 'application') to do more commands generate with `run`/`run-application` and `execution.target` check? or just `run` and `run-application` judgement?
Thanks.
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857666959
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
Yes, for this case with the `--run-mode`, we can reuse the test logic of line 27~34
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857561314
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
We only need to use `executeMode` to distinguish `run/run-application`. These means you only need to add two subType of `executeMode` enum, if you want to use enum.
This is only used for generate command `run/run-application`.
I think this way is more scalable, if we need to add another type like `run-xx` in the future. What do you think?
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
We only need to use `executeMode` to distinguish `run/run-application`. These means you only need to add two subType of `executeMode` enum, if you want to use enum.
This is only used for generate command `run/run-application`.
I think this way is more scalable, if we need to add another type like `run-xx` in the future. What do you think?
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857643156
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"-r", "--run-mode"},
+ description = "job run mode, run or run-application")
+ private String runMode;
Review Comment:
```suggestion
@Parameter(names = {"-r", "--run-mode"},
description = "job run mode, run or run-application",
required = false)
private String runMode = "run";
```
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857666959
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
Yes, for this case with the `--run-mode`, we can reuse the test logic of line 27~34
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#issuecomment-1108424665
Hi, @ruanwenjun , would you help review this PR ?
Thanks,
Kelu
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857521007
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
I would like to add a new parameter `private String executeMode` rather than add `isApplicationMode`. Or you can add a new enum `FlinkExecuteMode` to represent this.
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857740720
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
Yes, I just find in the old code may throw exception if user don't set `--run-mode`
```java
this.flinkCommandArgs = parseArgs(args);
String mode = flinkCommandArgs.getRunMode();
switch (mode) {
case RUN_MODE_RUN:
case RUN_MODE_APPLICATION:
this.runMode = mode;
break;
default:
throw new IllegalArgumentException("Run mode " + mode + " not supported");
}
```
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857672403
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"-r", "--run-mode"},
+ description = "job run mode, run or run-application")
+ private String runMode;
Review Comment:
@ruanwenjun I think optional will be a better user experience, as we already set the default value.
What do you thins?
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857561314
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
We only need to use `executeMode` to distinguish `run/run-application`. These means you only need to add two subType of `executeMode` enum, if you want to use enum.
I think this way is more scalable, if we need to add another type like `run-xx` in the future. What do you think?
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857563771
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
make sense.
I will update the PR lately.
Thanks.
--
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
[GitHub] [incubator-seatunnel] legendtkl commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857666959
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
Yes, for this case without the `--run-mode`, we can reuse the test logic of line 27~34
And without the `--run-mode`, we set it to default value `run`. What do you think?
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857610365
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
And don't forget to update the doc https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/command/usage.mdx
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857561314
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/command/FlinkCommandArgs.java:
##########
@@ -20,8 +20,14 @@
import org.apache.seatunnel.common.config.DeployMode;
import org.apache.seatunnel.config.EngineType;
+import com.beust.jcommander.Parameter;
+
public class FlinkCommandArgs extends AbstractCommandArgs {
+ @Parameter(names = {"--application-mode"},
+ description = "run job in application mode")
+ private boolean isApplicationMode = false;
Review Comment:
We only need to use `executeMode` to distinguish `run/run-application`. These means you only need to add two subType of `executeMode` enum, if you want to use enum.
This is only used for generating command `run/run-application`.
I think this way is more scalable, if we need to add another type like `run-xx` in the future. What do you think?
--
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
[GitHub] [incubator-seatunnel] ruanwenjun merged pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun merged PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742
--
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
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#issuecomment-1108568208
If we support flink Application mode, the #1722 may need do some change to support it.
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857648865
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
Please add test case without `--run-mode` parameter.
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#discussion_r857648865
##########
seatunnel-core/seatunnel-core-flink/src/test/java/org/apache/seatunnel/FlinkStarterTest.java:
##########
@@ -31,6 +31,23 @@ public void buildCommands() {
Assert.assertTrue(flinkExecuteCommand.contains("--config test.conf"));
Assert.assertTrue(flinkExecuteCommand.contains("-m yarn-cluster"));
Assert.assertTrue(flinkExecuteCommand.contains("-Dkey1=value1"));
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ String[] args1 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run-application"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args1).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run-application"));
+
+ String[] args2 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run"};
+ flinkExecuteCommand = String.join(" ", new FlinkStarter(args2).buildCommands());
+ Assert.assertTrue(flinkExecuteCommand.contains("${FLINK_HOME}/bin/flink run"));
+
+ try {
+ String[] args3 = {"--config", "test.conf", "-m", "yarn-cluster", "-i", "key1=value1", "-i", "key2=value2", "--run-mode", "run123"};
+ new FlinkStarter(args3);
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof IllegalArgumentException);
+ Assert.assertEquals("Run mode run123 not supported", e.getMessage());
+ }
Review Comment:
We can add test case without `--run-mode` parameter.
--
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
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1742: [Feature] [seatunnel-core-flink] flink job support application mode
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1742:
URL: https://github.com/apache/incubator-seatunnel/pull/1742#issuecomment-1108598805
> If we support flink Application mode, the #1722 may need do some change to support it.
Yes, in application mode, the `main` method will be executed at JobManager, then we cannot set the dependencies plugin by the current implementation. We may need to announce in the doc that user need to uploda plugins at hdfs and set the plugin path by `-Dyarn.provided.lib.dirs`
--
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