[GitHub] [hadoop] brumi1024 commented on a diff in pull request #5201: YARN-11393. Fs2cs could be extended to set ULF to -1 upon conversion

[GitBox <gi...@apache.org>]

brumi1024 commented on code in PR #5201:
URL: https://github.com/apache/hadoop/pull/5201#discussion_r1055653806


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/FSConfigToCSConfigConverter.java:
##########
@@ -412,6 +418,20 @@ private void emitDefaultMaxAMShare() {
           queueMaxAMShareDefault);
     }
   }
+
+  private void emitDefaultUserLimitFactor(Collection<FSLeafQueue> fsLeafQueue) {
+    fsLeafQueue
+        .forEach((leafQueue) -> {
+          if (!capacitySchedulerConfig.
+                isAutoQueueCreationV2Enabled(leafQueue.getName())) {
+            capacitySchedulerConfig.setFloat(
+                    CapacitySchedulerConfiguration.
+                            PREFIX + leafQueue.getName() + DOT + USER_LIMIT_FACTOR,
+                    -1.0f);
+          }
+        });
+  }
+

Review Comment:
   On the second look this class is more like for general config changes, while there is a class called FSQueueConverter, which has the queue specific converter logic implemented. Can you please move this whole logic there?



##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSConfigToCSConfigConverter.java:
##########
@@ -182,7 +183,24 @@ public void testDefaultMaxAMShare() throws Exception {
         conf.get(PREFIX + "root.admins.alice.maximum-am-resource-percent"));
 
     assertNull("root.users.joe maximum-am-resource-percent should be null",
-        conf.get(PREFIX + "root.users.joe maximum-am-resource-percent"));
+        conf.get(PREFIX + "root.users.joe.maximum-am-resource-percent"));
+  }
+
+  @Test
+  public void testDefaultUserLimitFactor() throws Exception {
+    converter.convert(config);
+
+    Configuration conf = converter.getCapacitySchedulerConfig();
+    String userLimitFactor =
+            conf.get(PREFIX + "root.default." + USER_LIMIT_FACTOR);
+
+    assertEquals("Default user limit factor", "-1.0", userLimitFactor);
+
+    assertEquals("root.users.joe user-limit-factor", "-1.0",
+            conf.get(PREFIX + "root.users.joe.user-limit-factor"));
+
+    assertEquals("root.admins.bob user-limit-factor", "-1.0",
+            conf.get(PREFIX + "root.admins.bob.user-limit-factor"));

Review Comment:
   Can you please add an assertion where a parent class doesn't get the user-limit-factor?



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

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org