[Issue-2299] Support the yarn queue label quick-setting for yarn application mode. #2317

RocMarshal · 2023-02-10T14:11:38Z

What changes were proposed in this pull request

Issue Number: close #2299

Brief change log

Support the yarn queue label quick-setting for yarn application mode.

Verifying this change

Add some unit test cases.
Manually verified the change by testing locally.

Does this pull request potentially affect one of the following parts

Dependencies (does it add or upgrade a dependency): (yes / no)

wolfboys · 2023-02-11T15:57:10Z

good job, Thanks for your contribution, I will review it later

RocMarshal · 2023-02-12T13:51:56Z

good job, Thanks for your contribution, I will review it later

Thank you @wolfboys . ping me if anything you need to communicate about .

RocMarshal · 2023-02-13T12:16:16Z

Hi, @wolfboys I added the note hints to the corresponding input, PTAL.
Thx~!

wolfboys · 2023-02-15T02:49:15Z

...service/src/main/java/org/apache/streampark/console/core/utils/YarnQueueLabelExpression.java

+/** Util class for parsing and checking Yarn queue & Label */
+public class YarnQueueLabelExpression {
+
+  public static final String AT = "@";


private is better.

wolfboys · 2023-02-15T02:49:30Z

...service/src/main/java/org/apache/streampark/console/core/utils/YarnQueueLabelExpression.java

+
+  public static final String AT = "@";
+
+  public static final String REGEX = "[a-zA-Z0-9_\\-]+";


wolfboys · 2023-02-15T02:50:52Z

...service/src/main/java/org/apache/streampark/console/core/utils/YarnQueueLabelExpression.java

+   * @param executionMode execution mode.
+   * @param queueLabel queueLabel expression.
+   */
+  public static void checkQueueLabelIfNeed(int executionMode, String queueLabel) {


how about?

public static void checkQueueLabelIfNeed(int executionMode, String queueLabel) { if (ExecutionMode.isYarnMode(executionMode)) { if (!YarnQueueLabelExpression.isValid(queueLabel, true)) { throw new IllegalArgumentException(ERR_HINTS); } } }

wolfboys · 2023-02-15T03:19:49Z

...service/src/main/java/org/apache/streampark/console/core/utils/YarnQueueLabelExpression.java

+   * @param queueLabelExp queue and label expression
+   * @param map the target properties.
+   */
+  public static <V> void addQueueLabelExprInto(String queueLabelExp, @Nonnull Map<String, V> map) {


Pass in a Map for filling. Not a good way, how about ?

public static Map<String, String> getQueueLabelMap(String queueLabelExp) { if (StringUtils.isEmpty(queueLabelExp)) { return Collections.emptyMap(); } YarnQueueLabelExpression yarnQueueLabelExpression = of(queueLabelExp); Map<String, String> map = new HashMap<>(2); yarnQueueLabelExpression .getLabelExpression() .ifPresent(labelExp -> map.put(ConfigConst.KEY_YARN_APP_NODE_LABEL(), labelExp)); map.put(ConfigConst.KEY_YARN_APP_QUEUE(), yarnQueueLabelExpression.queue); return map; }

Also I don't know if both "KEY_YARN_APP_NODE_LABEL" and "KEY_YARN_APP_QUEUE" need to put

If the label or queue exists, the corresponding value would be put.

If the label or queue exists, the corresponding value would be put.

get it

wolfboys · 2023-02-15T03:24:58Z

...ark-console-service/src/main/java/org/apache/streampark/console/core/entity/Application.java

@@ -598,9 +598,7 @@ public void updateHotParams(Application appParam) {
    ExecutionMode executionModeEnum = appParam.getExecutionModeEnum();
    Map<String, String> hotParams = new HashMap<>(0);
    if (ExecutionMode.YARN_APPLICATION.equals(executionModeEnum)) {


ExecutionMode.YARN_APPLICATION.equals(executionModeEnum) need to be changed to:
ExecutionMode.isYarnMode(executionModeEnum) This is a historical bug

oh, yarn-session execmode does not need to be set yarn queue, yarn-perjob is deprecated, but there should be many users who use, so. yarn-application and yarn-perjob need to be set yarn-queue

yarn-application and yarn-perjob need to be set yarn-queue

that's okay.

...ampark-console/streampark-console-webapp/src/views/flink/app/hooks/useCreateAndEditSchema.ts

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java

wolfboys · 2023-02-16T04:42:57Z

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java

-        }
-      }
-    }
+    fillBackYarnQueue(application);


method name : fillBackYarnQueue, how about change to setYarnQueue?

wolfboys

LGTM, thanks for your contribution

…ication mode. (apache#2317) * [Issue-2299] Introduce YarnQueueLabelExpression util class * [Issue-2299] Adapt the FE pages input placeholders * [Issue-2299] Adjust the backend logic to process label quick-setting * [Issue-2299] Support yarn session cluster queue label quick-setting

github-actions bot added BACKEND WEB UI labels Feb 10, 2023

RocMarshal marked this pull request as draft February 11, 2023 12:07

RocMarshal marked this pull request as ready for review February 11, 2023 13:23

RocMarshal mentioned this pull request Feb 13, 2023

[Issue-2299] Support yarn session cluster queue label quick-setting #2318

Closed

wolfboys reviewed Feb 15, 2023

View reviewed changes

...ampark-console/streampark-console-webapp/src/views/flink/app/hooks/useCreateAndEditSchema.ts Outdated Show resolved Hide resolved

RocMarshal requested a review from wolfboys February 15, 2023 12:24

wolfboys reviewed Feb 15, 2023

View reviewed changes

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java Show resolved Hide resolved

RocMarshal added 7 commits February 15, 2023 22:12

[Issue-2299] Introduce YarnQueueLabelExpression util class

a3f0d42

[Issue-2299] Adapt the FE pages input placeholders

2abedc5

[Issue-2299] Adjust the backend logic to process label quick-setting

40bee4b

[Issue-2299] Support yarn session cluster queue label quick-setting

0d7395a

Add Note hint for the input

f01db04

Updated based on review comments.

f6a38fb

Updated based on review comments.

4389237

RocMarshal commented Feb 15, 2023

View reviewed changes

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java Show resolved Hide resolved

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java Outdated Show resolved Hide resolved

wolfboys reviewed Feb 16, 2023

View reviewed changes

...ce/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java Outdated Show resolved Hide resolved

Update based on review comments

f46c367

wolfboys reviewed Feb 16, 2023

View reviewed changes

Rename method to setYarnQueue

3db2da3

wolfboys approved these changes Feb 16, 2023

View reviewed changes

wolfboys merged commit 02da4c3 into apache:dev Feb 16, 2023


		public static final String AT = "@";

		public static final String REGEX = "[a-zA-Z0-9_\\-]+";

[Issue-2299] Support the yarn queue label quick-setting for yarn application mode. #2317

[Issue-2299] Support the yarn queue label quick-setting for yarn application mode. #2317

Conversation

RocMarshal commented Feb 10, 2023

What changes were proposed in this pull request

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts

Uh oh!

wolfboys commented Feb 11, 2023

Uh oh!

RocMarshal commented Feb 12, 2023

Uh oh!

RocMarshal commented Feb 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfboys left a comment

Choose a reason for hiding this comment

Uh oh!