[GitHub] [flink] rkhachatryan commented on a diff in pull request #22288: [FLINK-31590] Add mechanics to the JobMaster for handling job resource requirements.

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

rkhachatryan commented on code in PR #22288:
URL: https://github.com/apache/flink/pull/22288#discussion_r1150621201


##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMaster.java:
##########
@@ -937,6 +944,34 @@ public CompletableFuture<Acknowledge> notifyNewBlockedNodes(Collection<BlockedNo
         return CompletableFuture.completedFuture(Acknowledge.get());
     }
 
+    @Override
+    public CompletableFuture<JobResourceRequirements> requestJobResourceRequirements() {
+        return CompletableFuture.completedFuture(schedulerNG.requestJobResourceRequirements());
+    }
+
+    @Override
+    public CompletableFuture<Acknowledge> updateJobResourceRequirements(
+            JobResourceRequirements jobResourceRequirements) {
+        final Map<JobVertexID, Integer> maxParallelismPerJobVertex = new HashMap<>();
+        for (JobVertex vertex : jobGraph.getVertices()) {
+            maxParallelismPerJobVertex.put(
+                    vertex.getID(), SchedulerBase.getDefaultMaxParallelism(vertex));
+        }
+        final List<String> validationErrors =
+                JobResourceRequirements.validate(
+                        jobResourceRequirements, maxParallelismPerJobVertex);
+        if (validationErrors.isEmpty()) {
+            schedulerNG.updateJobResourceRequirements(jobResourceRequirements);

Review Comment:
   What are the benefits of handling this error in DefaultScheduler over here?
   Something like 
   ```
           try {
               return CompletableFuture.completedFuture(schedulerNG.updateJobResourceRequirements(...));
           } catch (UnsupportedOperationException e) {
               return FutureUtils.completedExceptionally(
                   new RestHandlerException(e.getMessage(), HttpResponseStatus.NOT_IMPLEMENTED)); // not sure about the code
           }
   ```
   Handling `BAD_REQUEST` and `NOT_IMPLEMENTED` (?) in one place seems more consistent to me.



-- 
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@flink.apache.org

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