[bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode #14237

ORuteMa · 2023-05-30T02:43:28Z

Purpose of the pull request

Correct the way to determine the yarn queue in Flink CommandLine.

closed #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.

zhongjiajie · 2023-05-30T10:17:33Z

run ci

codecov-commenter · 2023-05-30T10:38:28Z

Codecov Report

Merging #14237 (065ed18) into dev (b8748e2) will increase coverage by 0.04%.
The diff coverage is 55.84%.

❗ Current head 065ed18 differs from pull request most recent head 406ce08. Consider uploading reports for the commit 406ce08 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14237      +/-   ##
============================================
+ Coverage     38.39%   38.43%   +0.04%     
- Complexity     4478     4502      +24     
============================================
  Files          1229     1235       +6     
  Lines         42936    43001      +65     
  Branches       4763     4767       +4     
============================================
+ Hits          16485    16528      +43     
- Misses        24625    24646      +21     
- Partials       1826     1827       +1

Impacted Files	Coverage Δ
...che/dolphinscheduler/api/python/PythonGateway.java	`16.94% <0.00%> (ø)`
...er/api/service/impl/MetricsCleanUpServiceImpl.java	`12.50% <0.00%> (-1.79%)`	⬇️
...api/service/impl/ProcessDefinitionServiceImpl.java	`35.35% <0.00%> (ø)`
...cheduler/common/constants/DataSourceConstants.java	`0.00% <ø> (ø)`
...in/datasource/vertica/VerticaDataSourceClient.java	`0.00% <0.00%> (ø)`
.../server/master/runner/StateWheelExecuteThread.java	`0.50% <ø> (+0.10%)`	⬆️
.../server/master/runner/WorkflowExecuteRunnable.java	`10.33% <ø> (+0.01%)`	⬆️
.../org/apache/dolphinscheduler/spi/enums/DbType.java	`0.00% <0.00%> (ø)`
...hinscheduler/plugin/task/flink/FlinkConstants.java	`0.00% <ø> (ø)`
...ler/server/worker/metrics/WorkerServerMetrics.java	`0.00% <0.00%> (ø)`
... and 15 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Radeity · 2023-05-30T11:48:15Z

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

-                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);
-                    }
-                }


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.

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

Radeity · 2023-05-30T13:19:25Z

...r-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
+


Please remove redundant blank line.

Radeity · 2023-05-30T13:19:36Z

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

                } else {
                    args.add(FlinkConstants.FLINK_RUN); // run
                    args.add(FlinkConstants.FLINK_RUN_MODE); // -m
                    args.add(FlinkConstants.FLINK_YARN_CLUSTER); // yarn-cluster
+


Radeity · 2023-05-30T13:19:40Z

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

                }
                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
+


Radeity · 2023-05-30T13:28:07Z

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

-            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)) {


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 ?

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.

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.

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?

Sure. From my practice, the queue is assigned. This logic should be correctly implemented in the current version.

It's weird, do you test on branch dev?

My dolphin env is 3.1.7

Have you tested the modification in this PR on branch dev?

I will test it tomorrow, in my env I cp this pr to my 3.1.7.

Radeity · 2023-05-30T13:35:16Z

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

+                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));
+                    }
+                }


There are some duplicated code. Can we clean up the logic and try to avoid introducing this new method?

Radeity · 2023-05-31T12:43:53Z

Hi, @ORuteMa , any feedback?

ORuteMa · 2023-06-01T03:27:54Z

Hi, @ORuteMa , any feedback?

Sorry I am busy these days, I will test it later.

ORuteMa · 2023-06-05T09:15:16Z

@caishunfeng @zhongjiajie pls approve run ci, thanks.

SbloodyS · 2023-06-05T10:30:54Z

@caishunfeng @zhongjiajie pls approve run ci, thanks.

Done.

ORuteMa · 2023-06-05T10:58:59Z

@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.

...r-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

+
+    private static void determinedYarnQueue(List<String> args, FlinkParameters flinkParameters,
+                                            FlinkDeployMode deployMode, String flinkVersion) {
+        switch (deployMode) {


zhongjiajie · 2023-06-08T05:59:11Z

restarted the failed E2E test

sonarcloud · 2023-06-08T06:28:10Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
1 Security Hotspot
13 Code Smells

10.4% Coverage
56.0% Duplication

zhongjiajie

LGTM, overall do you have addition suggestion? @Radeity

Radeity

+1

SbloodyS

+1

…k CommandLine and SQL mode (#14237) * Fix: Correct the way to determine the yarn queue in Flink CommandLine * fix the yarn queue in sql mode && refine the code * refine code * remove unnecessary comment * fix yarn queue properties * remove redundant variable

…k CommandLine and SQL mode (#14237) * Fix: Correct the way to determine the yarn queue in Flink CommandLine * fix the yarn queue in sql mode && refine the code * refine code * remove unnecessary comment * fix yarn queue properties * remove redundant variable (cherry picked from commit de2cc0e)

Fix: Correct the way to determine the yarn queue in Flink CommandLine

ac6f0a7

ORuteMa requested review from caishunfeng, SbloodyS and zhuangchong as code owners May 30, 2023 02:43

github-actions bot added the backend label May 30, 2023

zhongjiajie added the first time contributor First-time contributor label May 30, 2023

zhongjiajie self-requested a review May 30, 2023 09:46

ORuteMa changed the title ~~Fix: Correct the way to determine the yarn queue in Flink CommandLine~~ [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine May 30, 2023

Radeity requested changes May 30, 2023

View reviewed changes

fix the yarn queue in sql mode && refine the code

dcf290b

ORuteMa changed the title ~~[Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine~~ [Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode May 30, 2023

Radeity reviewed May 30, 2023

View reviewed changes

ORuteMa added 3 commits May 30, 2023 22:29

refine code

656d111

remove unnecessary comment

5f31443

fix yarn queue properties

1f276a9

Radeity mentioned this pull request May 30, 2023

[Bug] [Master] queue of taskExecutionContext is always null #14240

Closed

3 tasks

remove redundant variable

aae5b53

ORuteMa changed the title ~~[Bug][Plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode~~ [bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode May 30, 2023

caishunfeng assigned ORuteMa Jun 1, 2023

caishunfeng added bug Something isn't working 3.1.x for 3.1.x version labels Jun 1, 2023

run spotless apply

406ce08

ORuteMa mentioned this pull request Jun 5, 2023

[Feature][Task] Support yarn queue definition in yarn task #14251

Closed

3 tasks

github-advanced-security bot found potential problems Jun 5, 2023

View reviewed changes

zhongjiajie approved these changes Jun 9, 2023

View reviewed changes

Radeity approved these changes Jun 10, 2023

View reviewed changes

SbloodyS approved these changes Jun 12, 2023

View reviewed changes

SbloodyS added this to the 3.1.8 milestone Jun 12, 2023

SbloodyS merged commit de2cc0e into apache:dev Jun 12, 2023

zhuangchong added the release cherry-pick 3.1.x label Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode #14237

[bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode #14237

ORuteMa commented May 30, 2023 •

edited by zhongjiajie

Loading

zhongjiajie commented May 30, 2023

codecov-commenter commented May 30, 2023 •

edited

Loading

Radeity May 30, 2023

ORuteMa May 30, 2023

ORuteMa May 30, 2023

Radeity May 30, 2023

Radeity May 30, 2023

Radeity May 30, 2023

Radeity May 30, 2023 •

edited

Loading

ORuteMa May 30, 2023

ORuteMa May 30, 2023

Radeity May 30, 2023 •

edited

Loading

ORuteMa May 30, 2023

Radeity May 30, 2023

ORuteMa May 30, 2023

Radeity May 30, 2023

ORuteMa May 30, 2023

Radeity May 30, 2023

ORuteMa May 30, 2023

Radeity commented May 31, 2023

ORuteMa commented Jun 1, 2023

ORuteMa commented Jun 5, 2023

SbloodyS commented Jun 5, 2023

ORuteMa commented Jun 5, 2023

zhongjiajie commented Jun 8, 2023

sonarcloud bot commented Jun 8, 2023

zhongjiajie left a comment

Radeity left a comment

SbloodyS left a comment

[bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode #14237

[bug][plugin]Fix: Correct the way to determine the yarn queue in Flink CommandLine and SQL mode #14237

Conversation

ORuteMa commented May 30, 2023 • edited by zhongjiajie Loading

Purpose of the pull request

Brief change log

Verify this pull request

zhongjiajie commented May 30, 2023

codecov-commenter commented May 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radeity May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radeity May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radeity commented May 31, 2023

ORuteMa commented Jun 1, 2023

ORuteMa commented Jun 5, 2023

SbloodyS commented Jun 5, 2023

ORuteMa commented Jun 5, 2023

zhongjiajie commented Jun 8, 2023

sonarcloud bot commented Jun 8, 2023

zhongjiajie left a comment

Choose a reason for hiding this comment

Radeity left a comment

Choose a reason for hiding this comment

SbloodyS left a comment

Choose a reason for hiding this comment

ORuteMa commented May 30, 2023 •

edited by zhongjiajie

Loading

codecov-commenter commented May 30, 2023 •

edited

Loading

Radeity May 30, 2023 •

edited

Loading

Radeity May 30, 2023 •

edited

Loading