You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "ORuteMa (via GitHub)" <gi...@apache.org> on 2023/05/30 02:43:31 UTC
[GitHub] [dolphinscheduler] ORuteMa opened a new pull request, #14237: Fix: Correct the way to determine the yarn queue in Flink CommandLine
ORuteMa opened a new pull request, #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237
## Purpose of the pull request
Correct the way to determine the yarn queue in Flink CommandLine.
Close:https://github.com/apache/dolphinscheduler/issues/14236
## Brief change log
In Flink command line with -t option, Yarn queue should be determined by -Dyarn.application.name=%s rather than -yqu.
## Verify this pull request
This pull request is code cleanup without any test coverage. I test it manually.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210468862
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
I will test it tomorrow, in my env I cp this pr to my 3.1.7.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210273748
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
We will check whether user defines this arg by themselves in `others`, so you just have to modify condition in the original way: `!others.contains(FlinkConstants. FLINK_QUEUE_FOR_TARGETS)`, btw, may I ask why you name them `FLINK_QUEUE_FOR_MODE` and `FLINK_QUEUE_FOR_TARGETS `?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] SbloodyS merged pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210370687
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
It seems that this logic is missing, I've debugged and find the queue of `processInstance` is null, also in codes, I don't find where it's assigned by tenant's queue. Would like to help check in your local env?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210370687
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
It seems that this logic is missing, I've debugged and find the queue of `processInstance` is null. Also in codes, I don't find where it's assigned by tenant's queue. Would like to help check in your local env?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210449693
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
My dolphin env is 3.1.7
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210361651
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
The execution of a workflow is tied to a specific tenant, and this tenant holds a queue attribute. This attribute is assigned to the processInstance being executed by the tenant. The queue attribute of the executionContext for any taskInstance belonging to this processInstance will also be consistent. Therefore, the queue of a task is ultimately determined by the queue attribute of the runtime tenant if not explicitly specified.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #14237: Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1568211018
Please retry analysis of this Pull-Request directly on [SonarCloud](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237).
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210317090
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -306,6 +299,41 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(ParameterUtils.convertParameterPlaceholders(mainArgs, ParamUtils.convert(paramsMap)));
}
+ // determine yarn queue
+ determinedYarnQueue(args, flinkParameters, deployMode, flinkVersion);
return args;
}
+
+ private static void determinedYarnQueue(List<String> args, FlinkParameters flinkParameters,
+ FlinkDeployMode deployMode, String flinkVersion) {
+ String others = flinkParameters.getOthers();
+ switch (deployMode) {
+ case CLUSTER:
+ if (FLINK_VERSION_AFTER_OR_EQUALS_1_12.equals(flinkVersion)
+ || FLINK_VERSION_AFTER_OR_EQUALS_1_13.equals(flinkVersion)) {
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_TARGETS)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -Dyarn.application.queue=%s
+ args.add(String.format(FlinkConstants.FLINK_QUEUE_FOR_TARGETS, queue));
+ }
+ }
+ } else {
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_MODE)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -yqu
+ args.add(FlinkConstants.FLINK_QUEUE_FOR_MODE);
+ args.add(queue);
+ }
+ }
+ }
+ case APPLICATION:
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_TARGETS)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -Dyarn.application.queue=%s
+ args.add(String.format(FlinkConstants.FLINK_QUEUE_FOR_TARGETS, queue));
+ }
+ }
Review Comment:
Sure
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210155641
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -254,13 +279,6 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(taskManagerMemory);
}
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) { // -yqu
- args.add(FlinkConstants.FLINK_QUEUE);
- args.add(queue);
- }
- }
Review Comment:
I think you should add if-else statement here to set different arg name for yarn queue according to Flink version. Please remove judgement from L195-L244, in which just build run command.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] github-code-scanning[bot] commented on a diff in pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1217908582
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -306,6 +296,40 @@
args.add(ParameterUtils.convertParameterPlaceholders(mainArgs, ParamUtils.convert(paramsMap)));
}
+ // determine yarn queue
+ determinedYarnQueue(args, flinkParameters, deployMode, flinkVersion);
return args;
}
+
+ private static void determinedYarnQueue(List<String> args, FlinkParameters flinkParameters,
+ FlinkDeployMode deployMode, String flinkVersion) {
+ switch (deployMode) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [STANDALONE](1).
Switch statement does not have a case for [LOCAL](2).
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/2947)
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1571273700
> Hi, @ORuteMa , any feedback?
Sorry I am busy these days, I will test it later.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210343942
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
It seems that we don't have any option for setting yarn queue, `flinkParameters.queue` will be assigned by `taskExecutionContext.queue`, which pass from `processInstance`, however, it's never be assigned.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1570165340
Hi, @ORuteMa , any feedback?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1576430684
@caishunfeng @zhongjiajie pls approve run ci, 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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1576573824
> > @caishunfeng @zhongjiajie pls approve run ci, thanks.
>
> Done.
I see some fail in this run, it may have no business to do with my pr. Is it a network problem or something else? Pls have a look.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210439058
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
Sure. From my practice, the queue is assigned. This logic should be correctly implemented in the current version.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210343942
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
It seems that we don't have any option for setting yarn queue, `flinkParameters.queue` will be assigned by `taskExecutionContext.queue`, which pass from `processInstance`, however, it's never be assigned.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on pull request #14237: Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "zhongjiajie (via GitHub)" <gi...@apache.org>.
zhongjiajie commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1568175019
run ci
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter commented on pull request #14237: Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1568201766
## [Codecov](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#14237](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2f88f1f) into [dev](https://app.codecov.io/gh/apache/dolphinscheduler/commit/b8748e29ae27c48b6bd37ccc0bd198384c586ba1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b8748e2) will **decrease** coverage by `0.05%`.
> The diff coverage is `38.88%`.
> :exclamation: Current head 2f88f1f differs from pull request most recent head ac6f0a7. Consider uploading reports for the commit ac6f0a7 to get more accurate results
```diff
@@ Coverage Diff @@
## dev #14237 +/- ##
============================================
- Coverage 38.39% 38.34% -0.05%
+ Complexity 4478 4468 -10
============================================
Files 1229 1229
Lines 42936 42896 -40
Branches 4763 4737 -26
============================================
- Hits 16485 16450 -35
+ Misses 24625 24622 -3
+ Partials 1826 1824 -2
```
| [Impacted Files](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...hinscheduler/plugin/task/flink/FlinkConstants.java](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZG9scGhpbnNjaGVkdWxlci10YXNrLXBsdWdpbi9kb2xwaGluc2NoZWR1bGVyLXRhc2stZmxpbmsvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcGx1Z2luL3Rhc2svZmxpbmsvRmxpbmtDb25zdGFudHMuamF2YQ==) | `0.00% <ø> (ø)` | |
| [...hinscheduler/plugin/task/flink/FlinkArgsUtils.java](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZG9scGhpbnNjaGVkdWxlci10YXNrLXBsdWdpbi9kb2xwaGluc2NoZWR1bGVyLXRhc2stZmxpbmsvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcGx1Z2luL3Rhc2svZmxpbmsvRmxpbmtBcmdzVXRpbHMuamF2YQ==) | `61.70% <38.88%> (-2.21%)` | :arrow_down: |
... and [39 files with indirect coverage changes](https://app.codecov.io/gh/apache/dolphinscheduler/pull/14237/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] SbloodyS commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1576539400
> @caishunfeng @zhongjiajie pls approve run ci, thanks.
Done.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] zhongjiajie commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "zhongjiajie (via GitHub)" <gi...@apache.org>.
zhongjiajie commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1581936141
restarted the failed E2E test
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1581963233
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=14237)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list) [10.4% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list) [56.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list)
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210157639
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -254,13 +279,6 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(taskManagerMemory);
}
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) { // -yqu
- args.add(FlinkConstants.FLINK_QUEUE);
- args.add(queue);
- }
- }
Review Comment:
ok
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210454139
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
Have you tested the modification in this PR on branch dev?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210262143
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -197,16 +196,19 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(FlinkConstants.FLINK_RUN); // run
args.add(FlinkConstants.FLINK_EXECUTION_TARGET); // -t
args.add(FlinkConstants.FLINK_YARN_PER_JOB); // yarn-per-job
+
Review Comment:
Please remove redundant blank line.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -197,16 +196,19 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(FlinkConstants.FLINK_RUN); // run
args.add(FlinkConstants.FLINK_EXECUTION_TARGET); // -t
args.add(FlinkConstants.FLINK_YARN_PER_JOB); // yarn-per-job
+
} else {
args.add(FlinkConstants.FLINK_RUN); // run
args.add(FlinkConstants.FLINK_RUN_MODE); // -m
args.add(FlinkConstants.FLINK_YARN_CLUSTER); // yarn-cluster
+
Review Comment:
Ditto.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
We will check whether user defines this arg by themselves in `others`, so you just have to add one condition in the original way: `!others.contains(FlinkConstants. FLINK_QUEUE_FOR_TARGETS)`, btw, may I ask why you name them `FLINK_QUEUE_FOR_MODE` and `FLINK_QUEUE_FOR_TARGETS `?
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -197,16 +196,19 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(FlinkConstants.FLINK_RUN); // run
args.add(FlinkConstants.FLINK_EXECUTION_TARGET); // -t
args.add(FlinkConstants.FLINK_YARN_PER_JOB); // yarn-per-job
+
} else {
args.add(FlinkConstants.FLINK_RUN); // run
args.add(FlinkConstants.FLINK_RUN_MODE); // -m
args.add(FlinkConstants.FLINK_YARN_CLUSTER); // yarn-cluster
+
}
break;
case APPLICATION:
args.add(FlinkConstants.FLINK_RUN_APPLICATION); // run-application
args.add(FlinkConstants.FLINK_EXECUTION_TARGET); // -t
args.add(FlinkConstants.FLINK_YARN_APPLICATION); // yarn-application
+
Review Comment:
Ditto.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -306,6 +299,41 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(ParameterUtils.convertParameterPlaceholders(mainArgs, ParamUtils.convert(paramsMap)));
}
+ // determine yarn queue
+ determinedYarnQueue(args, flinkParameters, deployMode, flinkVersion);
return args;
}
+
+ private static void determinedYarnQueue(List<String> args, FlinkParameters flinkParameters,
+ FlinkDeployMode deployMode, String flinkVersion) {
+ String others = flinkParameters.getOthers();
+ switch (deployMode) {
+ case CLUSTER:
+ if (FLINK_VERSION_AFTER_OR_EQUALS_1_12.equals(flinkVersion)
+ || FLINK_VERSION_AFTER_OR_EQUALS_1_13.equals(flinkVersion)) {
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_TARGETS)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -Dyarn.application.queue=%s
+ args.add(String.format(FlinkConstants.FLINK_QUEUE_FOR_TARGETS, queue));
+ }
+ }
+ } else {
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_MODE)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -yqu
+ args.add(FlinkConstants.FLINK_QUEUE_FOR_MODE);
+ args.add(queue);
+ }
+ }
+ }
+ case APPLICATION:
+ if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE_FOR_TARGETS)) {
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) { // -Dyarn.application.queue=%s
+ args.add(String.format(FlinkConstants.FLINK_QUEUE_FOR_TARGETS, queue));
+ }
+ }
Review Comment:
There are some duplicated code. Can we clean up the logic and try to avoid introducing this new method?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #14237: [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1581961457
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=14237)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [14 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list) [10.4% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list) [56.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list)
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] Radeity commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210448343
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
It's weird, do you test on branch dev?
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #14237: Fix: Correct the way to determine the yarn queue in Flink CommandLine
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#issuecomment-1568210257
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=14237)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=14237&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list) [52.5% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=14237&metric=new_duplicated_lines_density&view=list)
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210253422
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -254,13 +279,6 @@ private static List<String> buildRunCommandLineForOthers(TaskExecutionContext ta
args.add(taskManagerMemory);
}
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) { // -yqu
- args.add(FlinkConstants.FLINK_QUEUE);
- args.add(queue);
- }
- }
Review Comment:
> I think you should add if-else statement here to set different arg name for yarn queue according to Flink version. Please remove judgement from L195-L244, in which just build run command.
done, and fix the same wrong in sql mode, pls have a look
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ORuteMa commented on a diff in pull request #14237: [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode
Posted by "ORuteMa (via GitHub)" <gi...@apache.org>.
ORuteMa commented on code in PR #14237:
URL: https://github.com/apache/dolphinscheduler/pull/14237#discussion_r1210315301
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java:
##########
@@ -164,12 +164,9 @@ public static List<String> buildInitOptionsForSql(FlinkParameters flinkParameter
}
// yarn.application.queue
- String others = flinkParameters.getOthers();
- if (StringUtils.isEmpty(others) || !others.contains(FlinkConstants.FLINK_QUEUE)) {
- String queue = flinkParameters.getQueue();
- if (StringUtils.isNotEmpty(queue)) {
- initOptions.add(String.format(FlinkConstants.FLINK_FORMAT_YARN_APPLICATION_QUEUE, queue));
- }
+ String queue = flinkParameters.getQueue();
+ if (StringUtils.isNotEmpty(queue)) {
Review Comment:
The YARN queue should be assigned by property `yarn.application.queue` rather than -yqu option, this option is not available in sql-client.sh. If we want to specify the YARN queue used by a specific Flink SQL task, it would be more appropriate to have an explicit queue option in the task submission form rather than relying on parameters in the 'others' section. Regarding the naming here, it is because in flink-run, the -yqu option only takes effect within the -m option (for mode). When using the -t option (for target), it is necessary to specify it using -Dyarn.application.queue=%s.
--
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@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org