[GitHub] [incubator-streampark] wolfboys commented on a diff in pull request #2448: [Feature][Issue-2324/Issue-2420] Support selection on yarn queue when create/edit yarn per-job/application-mode application & yarn-session cluster.

["wolfboys (via GitHub)" <gi...@apache.org>]

wolfboys commented on code in PR #2448:
URL: https://github.com/apache/incubator-streampark/pull/2448#discussion_r1161286972


##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/FlinkClusterServiceImpl.java:
##########
@@ -356,4 +361,49 @@ public void delete(FlinkCluster cluster) {
     }
     removeById(id);
   }
+
+  /** Check queue label validation when create the cluster if needed. */
+  @VisibleForTesting
+  public void checkQueueValidationIfNeeded(FlinkCluster clusterInfo) {
+    yarnQueueService.checkQueueLabelFormatIfNeeded(
+        clusterInfo.getExecutionModeEnum(), clusterInfo.getYarnQueue());
+    if (!isYarnSessionModeAndNotDefaultQueue(clusterInfo)) {
+      return;
+    }
+    boolean exist = yarnQueueService.existByQueueLabel(clusterInfo.getYarnQueue());
+    ApiAlertException.throwIfFalse(
+        exist, String.format(ERROR_CLUSTER_QUEUE_HINT, clusterInfo.getYarnQueue()));
+  }
+
+  /** Check queue label validation when update the cluster if needed. */
+  @VisibleForTesting
+  public void checkQueueValidationIfNeeded(FlinkCluster oldCluster, FlinkCluster newCluster) {
+    yarnQueueService.checkQueueLabelFormatIfNeeded(
+        newCluster.getExecutionModeEnum(), newCluster.getYarnQueue());
+    if (!isYarnSessionModeAndNotDefaultQueue(newCluster)) {
+      return;
+    }
+
+    if (ExecutionMode.isYarnSessionMode(newCluster.getExecutionModeEnum())
+        && StringUtils.equals(oldCluster.getYarnQueue(), newCluster.getYarnQueue())) {
+      return;
+    }
+    boolean exist = yarnQueueService.existByQueueLabel(newCluster.getYarnQueue());
+    ApiAlertException.throwIfFalse(
+        exist, String.format(ERROR_CLUSTER_QUEUE_HINT, newCluster.getYarnQueue()));
+  }
+
+  /**
+   * Judge the execution mode whether is the Yarn session mode with not default or empty queue
+   * label.
+   *
+   * @param cluster cluster.
+   * @return If the executionMode is yarn session mode and the queue label is not (empty or
+   *     default), return true, false else.
+   */
+  @VisibleForTesting
+  public boolean isYarnSessionModeAndNotDefaultQueue(FlinkCluster cluster) {
+    return ExecutionMode.isYarnSessionMode(cluster.getExecutionModeEnum())
+        && !yarnQueueService.isEmptyOrDefaultQueue(cluster.getYarnQueue());
+  }

Review Comment:
   Would it be better to rename `isEmptyOrDefaultQueue` to `isDefaultQueue` ?



##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/YarnQueueServiceImpl.java:
##########
@@ -166,6 +168,40 @@ public void deleteYarnQueue(YarnQueue yarnQueue) {
     removeById(yarnQueue.getId());
   }
 
+  /**
+   * Only check the validation of queue-labelExpression when using yarn application or yarn-session
+   * mode or yarn-perjob mode.
+   *
+   * @param executionMode execution mode.
+   * @param queueLabel queueLabel expression.
+   */
+  @Override
+  public void checkQueueLabelFormatIfNeeded(ExecutionMode executionMode, String queueLabel) {
+    if (ExecutionMode.isYarnMode(executionMode)) {
+      ApiAlertException.throwIfFalse(isValid(queueLabel, true), ERR_FORMAT_HINTS);

Review Comment:
   I suggest not to throw exception here, just return the result, it's up to the caller to decide whether it needs to throw an exception



##########
streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/core/service/impl/ApplicationServiceImpl.java:
##########
@@ -1724,4 +1731,52 @@ private String getSavePointed(Application appParam) {
     }
     return null;
   }
+
+  /** Check queue label validation when create the application if needed. */
+  @VisibleForTesting
+  public void checkQueueValidationIfNeeded(Application appParam) {
+    yarnQueueService.checkQueueLabelFormatIfNeeded(
+        appParam.getExecutionModeEnum(), appParam.getYarnQueue());
+    if (!isYarnPerJobAppModeNotDefaultQueue(appParam)) {
+      return;
+    }
+    boolean exist =
+        yarnQueueService.existByTeamIdQueueLabel(appParam.getTeamId(), appParam.getYarnQueue());
+    ApiAlertException.throwIfFalse(
+        exist, String.format(ERROR_APP_QUEUE_HINT, appParam.getYarnQueue(), appParam.getTeamId()));
+  }
+
+  /** Check queue label validation when update the application if needed. */
+  @VisibleForTesting
+  public void checkQueueValidationIfNeeded(Application oldApp, Application newApp) {
+    yarnQueueService.checkQueueLabelFormatIfNeeded(
+        newApp.getExecutionModeEnum(), newApp.getYarnQueue());
+    if (!isYarnPerJobAppModeNotDefaultQueue(newApp)) {
+      return;
+    }
+
+    oldApp.setYarnQueueByHotParams();
+    if (ExecutionMode.isYarnPerJobOrAppMode(newApp.getExecutionModeEnum())
+        && StringUtils.equals(oldApp.getYarnQueue(), newApp.getYarnQueue())) {
+      return;
+    }
+    boolean exist =
+        yarnQueueService.existByTeamIdQueueLabel(newApp.getTeamId(), newApp.getYarnQueue());
+    ApiAlertException.throwIfFalse(
+        exist, String.format(ERROR_APP_QUEUE_HINT, newApp.getYarnQueue(), newApp.getTeamId()));
+  }
+
+  /**
+   * Judge the execution mode whether is the Yarn PerJob or Application mode with not default or
+   * empty queue label.
+   *
+   * @param application application entity.
+   * @return If the executionMode is (Yarn PerJob or application mode) and the queue label is not
+   *     (empty or default), return true, false else.
+   */
+  @VisibleForTesting
+  public boolean isYarnPerJobAppModeNotDefaultQueue(Application application) {

Review Comment:
   We can change `public` to `private` and method rename to `isYarnNotDefaultQueue`



-- 
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: issues-unsubscribe@streampark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org