[GitHub] [hadoop] 9uapaw commented on a change in pull request #3550: YARN-10907. Minimize usages of AbstractCSQueue#csContext

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

9uapaw commented on a change in pull request #3550:
URL: https://github.com/apache/hadoop/pull/3550#discussion_r740835594



##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/monitor/capacity/ProportionalCapacityPreemptionPolicy.java
##########
@@ -791,11 +791,7 @@ public Resource getPartitionResource(String partition) {
   public void addPartitionToUnderServedQueues(String queueName,
       String partition) {
     LinkedHashSet<String> underServedQueues = partitionToUnderServedQueues
-        .get(partition);
-    if (null == underServedQueues) {
-      underServedQueues = new LinkedHashSet<String>();
-      partitionToUnderServedQueues.put(partition, underServedQueues);
-    }
+        .computeIfAbsent(partition, k -> new LinkedHashSet<>());

Review comment:
       Nit: I think the complaint of IntelliJ is invalid here, and LinkedHashSet<String>::new is allowed here instead of the lambda.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -152,4 +159,8 @@ protected Resource getReservationQueueResourceIfExists(Plan plan,
     return reservationResource;
   }
 
+  @Override
+  protected String getReservationIdFromQueueName(String resQueueName) {
+    return resQueueName.substring(resQueueName.lastIndexOf(".") + 1);

Review comment:
       I think queueName is more like a queuePath here. Also, the QueuePath class has getLeafName method, which is probably what you want here.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
##########
@@ -31,26 +31,25 @@
 public class QueueNodeLabelsSettings {
   private final CSQueue parent;
   private final String queuePath;
-  private final CapacitySchedulerContext csContext;
   private Set<String> accessibleLabels;
   private Set<String> configuredNodeLabels;
   private String defaultLabelExpression;
 
   public QueueNodeLabelsSettings(CapacitySchedulerConfiguration configuration,
       CSQueue parent,
       String queuePath,
-      CapacitySchedulerContext csContext) throws IOException {
+      ConfiguredNodeLabels configuredNodeLabelsParam) throws IOException {

Review comment:
       Nit: I think just ignore the checkstyle warning and name this as configuredNodeLabels.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueuePreemptionSettings.java
##########
@@ -40,14 +42,14 @@ public CSQueuePreemptionSettings(
    * NOTE: Cross-queue preemptability is inherited from a queue's parent.
    *
    * @param q queue to check preemption state
-   * @param csContext
    * @param configuration capacity scheduler config
    * @return true if queue has cross-queue preemption disabled, false otherwise
    */
   private boolean isQueueHierarchyPreemptionDisabled(CSQueue q,
-      CapacitySchedulerContext csContext, CapacitySchedulerConfiguration configuration) {
+      CapacitySchedulerConfiguration configuration,
+      CapacitySchedulerConfiguration originalSchedulerConfiguration) {

Review comment:
       I have failed to reason about the concept of configuration vs originalSchedulerConfiguration. Is it truly necessary to have both of them? If yes, please add some documentation in which you explain its necessity.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -114,8 +116,13 @@ protected void createDefaultReservationQueue(
     PlanQueue planQueue = (PlanQueue)queue;
     if (cs.getQueue(defReservationId) == null) {
       try {
+        CSAutoCreatedLeafQueueProperties queueProperties = new CSAutoCreatedLeafQueueProperties(
+            planQueue.getQueuePath() + "." + defReservationId,
+            cs.getMinimumResourceCapability());
+
         ReservationQueue defQueue =
-            new ReservationQueue(cs, defReservationId, planQueue);
+            new ReservationQueue(cs.getCapacitySchedulerQueueManager(),
+                queueProperties, planQueue);
         cs.addQueue(defQueue);
       } catch (SchedulerDynamicEditException e) {
         LOG.warn(

Review comment:
       This block shares some similarities with the block in addReservationQueue. It could be extracted.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ReservationQueue.java
##########
@@ -37,9 +37,17 @@
 
   private PlanQueue parent;
 
-  public ReservationQueue(CapacitySchedulerContext cs, String queueName,
+  public ReservationQueue(CapacitySchedulerQueueManager queueManager,
+      CSAutoCreatedLeafQueueProperties autoCreatedLeafQueueProperties,
       PlanQueue parent) throws IOException {
-    super(cs, queueName, parent, null);
+    super(queueManager, autoCreatedLeafQueueProperties, parent, null);
+    super.setupQueueConfigs(queueManager.getClusterResource(),
+        queueManager.getSchedulerConfiguration());
+
+    LOG.debug("Initialized ReservationQueue: name={}, fullname={}",

Review comment:
       This is a recurring idiom, could be extracted to the super class and use the name of the concrete class there.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceConfiguration.java
##########
@@ -211,7 +211,10 @@ public void testSimpleMinMaxResourceConfigurartionPerQueue()
     CapacityScheduler cs = (CapacityScheduler) rm.getResourceScheduler();
 
     ManagedParentQueue parentQueue = (ManagedParentQueue) cs.getQueue(QUEUED);
-    AutoCreatedLeafQueue d1 = new AutoCreatedLeafQueue(cs, "d1", parentQueue);
+    CSAutoCreatedLeafQueueProperties queuePropertiesD1 = new CSAutoCreatedLeafQueueProperties(
+        parentQueue.getQueuePath() + ".d1", cs.getMinimumAllocation());

Review comment:
       Please use the new QueuePath class here.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -94,14 +95,15 @@ protected void addReservationQueue(
       String planQueueName, Queue queue, String currResId) {
     PlanQueue planQueue = (PlanQueue)queue;
     try {
+      CSAutoCreatedLeafQueueProperties autoCreatedLeafQueueProperties =
+          new CSAutoCreatedLeafQueueProperties(planQueue.getQueuePath() + "."

Review comment:
       Using the createNewLeaf method of the new QueuePath class would be better here, as it is the same logic.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimits.java
##########
@@ -119,24 +120,32 @@ public void setUp() throws IOException {
     when(csContext.getResourceCalculator()).
         thenReturn(resourceCalculator);
     when(csContext.getRMContext()).thenReturn(rmContext);
-    when(csContext.getPreemptionManager()).thenReturn(new PreemptionManager());
-    
+    when(csContext.getPreemptionManager()).thenReturn(preemptionManager);
+
+    CapacitySchedulerQueueManager queueManager = new CapacitySchedulerQueueManager(csContext,
+        rmContext.getNodeLabelManager(), preemptionManager, null,
+        null);
+    when(csContext.getCapacitySchedulerQueueManager()).thenReturn(queueManager);
+
     RMContainerTokenSecretManager containerTokenSecretManager =
         new RMContainerTokenSecretManager(conf);
     containerTokenSecretManager.rollMasterKey();
     when(csContext.getContainerTokenSecretManager()).thenReturn(
         containerTokenSecretManager);
 
     CSQueueStore queues = new CSQueueStore();
-    root = CapacitySchedulerQueueManager
-        .parseQueue(csContext, csConf, null, "root",
-            queues, queues,
-            TestUtils.spyHook);
+    root = queueManager.parseQueue(null, "root",
+            queues, queues, TestUtils.spyHook);
+    queueManager.setRootQueue(root);
     root.updateClusterResource(clusterResource,
         new ResourceLimits(clusterResource));
 
-    queue = spy(new LeafQueue(csContext, A, root, null));
-    QueueResourceQuotas queueResourceQuotas = ((LeafQueue) queues.get(A))
+    CSLeafQueueProperties csQueuePropertiesA =
+        new CSLeafQueueProperties(root.getQueuePath() + "." + A,

Review comment:
       See previous comment.

##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -114,8 +116,13 @@ protected void createDefaultReservationQueue(
     PlanQueue planQueue = (PlanQueue)queue;
     if (cs.getQueue(defReservationId) == null) {
       try {
+        CSAutoCreatedLeafQueueProperties queueProperties = new CSAutoCreatedLeafQueueProperties(
+            planQueue.getQueuePath() + "." + defReservationId,

Review comment:
       See comment before.




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