YARN-9318. Resources#multiplyAndRoundUp does not consider Resource Types

(Contributed by Szilard Nemeth via Daniel Templeton)

Change-Id: Ia45f528574c2b054f6f764d1d140e592bdb7e217

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/resource/Resources.java

@@ -35,6 +35,8 @@
 @Unstable
 public class Resources {
 
+  private enum RoundingDirection { UP, DOWN }
+
   private static final Log LOG =
       LogFactory.getLog(Resources.class);
 
@@ -305,17 +307,7 @@ public static Resource negate(Resource resource) {
   }
 
   public static Resource multiplyTo(Resource lhs, double by) {
-    int maxLength = ResourceUtils.getNumberOfCountableResourceTypes();
+    return multiplyAndRound(lhs, by, RoundingDirection.DOWN);
-    for (int i = 0; i < maxLength; i++) {
-      try {
-        ResourceInformation lhsValue = lhs.getResourceInformation(i);
-        lhs.setResourceValue(i, (long) (lhsValue.getValue() * by));
-      } catch (ResourceNotFoundException ye) {
-        LOG.warn("Resource is missing:" + ye.getMessage());
-        continue;
-      }
-    }
-    return lhs;
   }
 
   public static Resource multiply(Resource lhs, double by) {
@@ -338,7 +330,6 @@ public static Resource multiplyAndAddTo(
         lhs.setResourceValue(i, lhsValue.getValue() + convertedRhs);
       } catch (ResourceNotFoundException ye) {
         LOG.warn("Resource is missing:" + ye.getMessage());
-        continue;
       }
     }
     return lhs;
@@ -359,26 +350,55 @@ public static Resource multiplyAndNormalizeDown(
     return calculator.multiplyAndNormalizeDown(lhs, by, factor);
   }
 
+  /**
+   * Multiply {@code lhs} by {@code by}, and set the result rounded down into a
+   * cloned version of {@code lhs} Resource object.
+   * @param lhs Resource object
+   * @param by Multiply values by this value
+   * @return A cloned version of {@code lhs} with updated values
+   */
   public static Resource multiplyAndRoundDown(Resource lhs, double by) {
-    Resource out = clone(lhs);
+    return multiplyAndRound(clone(lhs), by, RoundingDirection.DOWN);
+  }
+
+  /**
+   * Multiply {@code lhs} by {@code by}, and set the result rounded up into a
+   * cloned version of {@code lhs} Resource object.
+   * @param lhs Resource object
+   * @param by Multiply values by this value
+   * @return A cloned version of {@code lhs} with updated values
+   */
+  public static Resource multiplyAndRoundUp(Resource lhs, double by) {
+    return multiplyAndRound(clone(lhs), by, RoundingDirection.UP);
+  }
+
+  /**
+   * Multiply {@code lhs} by {@code by}, and set the result according to
+   * the rounding direction to {@code lhs}
+   * without creating any new {@link Resource} object.
+   * @param lhs Resource object
+   * @param by Multiply values by this value
+   * @return Returns {@code lhs} itself (without cloning) with updated values
+   */
+  private static Resource multiplyAndRound(Resource lhs, double by,
+      RoundingDirection roundingDirection) {
     int maxLength = ResourceUtils.getNumberOfCountableResourceTypes();
     for (int i = 0; i < maxLength; i++) {
       try {
         ResourceInformation lhsValue = lhs.getResourceInformation(i);
-        out.setResourceValue(i, (long) (lhsValue.getValue() * by));
+
+        final long value;
+        if (roundingDirection == RoundingDirection.DOWN) {
+          value = (long) (lhsValue.getValue() * by);
+        } else {
+          value = (long) Math.ceil(lhsValue.getValue() * by);
+        }
+        lhs.setResourceValue(i, value);
       } catch (ResourceNotFoundException ye) {
         LOG.warn("Resource is missing:" + ye.getMessage());
-        continue;
       }
     }
-    return out;
+    return lhs;
-  }
-
-  public static Resource multiplyAndRoundUp(Resource lhs, double by) {
-    Resource out = clone(lhs);
-    out.setMemorySize((long)Math.ceil(lhs.getMemorySize() * by));
-    out.setVirtualCores((int)Math.ceil(lhs.getVirtualCores() * by));
-    return out;
   }
 
   public static Resource normalize(

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/resource/TestResources.java

@@ -31,6 +31,7 @@
 import static org.apache.hadoop.yarn.util.resource.Resources.componentwiseMin;
 import static org.apache.hadoop.yarn.util.resource.Resources.componentwiseMax;
 import static org.apache.hadoop.yarn.util.resource.Resources.add;
+import static org.apache.hadoop.yarn.util.resource.Resources.multiplyAndRoundUp;
 import static org.apache.hadoop.yarn.util.resource.Resources.subtract;
 import static org.apache.hadoop.yarn.util.resource.Resources.multiply;
 import static org.apache.hadoop.yarn.util.resource.Resources.multiplyAndAddTo;
@@ -41,6 +42,7 @@
 import static org.junit.Assert.assertTrue;
 
 public class TestResources {
+  private static final String INVALID_RESOURCE_MSG = "Invalid resource value";
 
   static class ExtendedResources extends Resources {
     public static Resource unbounded() {
@@ -116,27 +118,6 @@ public void testCompareToWithNoneResource() {
     assertTrue(Resources.none().compareTo(createResource(0, 0, 1)) < 0);
   }
 
-  @Test(timeout=10000)
-  public void testMultipleRoundUp() {
-    final double by = 0.5;
-    final String memoryErrorMsg = "Invalid memory size.";
-    final String vcoreErrorMsg = "Invalid virtual core number.";
-    Resource resource = Resources.createResource(1, 1);
-    Resource result = Resources.multiplyAndRoundUp(resource, by);
-    assertEquals(memoryErrorMsg, result.getMemorySize(), 1);
-    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 1);
-
-    resource = Resources.createResource(2, 2);
-    result = Resources.multiplyAndRoundUp(resource, by);
-    assertEquals(memoryErrorMsg, result.getMemorySize(), 1);
-    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 1);
-
-    resource = Resources.createResource(0, 0);
-    result = Resources.multiplyAndRoundUp(resource, by);
-    assertEquals(memoryErrorMsg, result.getMemorySize(), 0);
-    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 0);
-  }
-
   @Test(timeout = 1000)
   public void testFitsIn() {
     assertTrue(fitsIn(createResource(1, 1), createResource(2, 2)));
@@ -228,19 +209,56 @@ public void testMultiply() {
     assertEquals(createResource(4, 4, 6), multiply(createResource(2, 2, 3), 2));
   }
 
+  @Test(timeout=10000)
+  public void testMultiplyRoundUp() {
+    final double by = 0.5;
+    final String memoryErrorMsg = "Invalid memory size.";
+    final String vcoreErrorMsg = "Invalid virtual core number.";
+    Resource resource = Resources.createResource(1, 1);
+    Resource result = Resources.multiplyAndRoundUp(resource, by);
+    assertEquals(memoryErrorMsg, result.getMemorySize(), 1);
+    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 1);
+
+    resource = Resources.createResource(2, 2);
+    result = Resources.multiplyAndRoundUp(resource, by);
+    assertEquals(memoryErrorMsg, result.getMemorySize(), 1);
+    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 1);
+
+    resource = Resources.createResource(0, 0);
+    result = Resources.multiplyAndRoundUp(resource, by);
+    assertEquals(memoryErrorMsg, result.getMemorySize(), 0);
+    assertEquals(vcoreErrorMsg, result.getVirtualCores(), 0);
+  }
+
+  @Test
+  public void testMultiplyAndRoundUpCustomResources() {
+    assertEquals(INVALID_RESOURCE_MSG, createResource(5, 2, 8),
+        multiplyAndRoundUp(createResource(3, 1, 5), 1.5));
+    assertEquals(INVALID_RESOURCE_MSG, createResource(5, 2, 0),
+        multiplyAndRoundUp(createResource(3, 1, 0), 1.5));
+    assertEquals(INVALID_RESOURCE_MSG, createResource(5, 5, 0),
+        multiplyAndRoundUp(createResource(3, 3, 0), 1.5));
+    assertEquals(INVALID_RESOURCE_MSG, createResource(8, 3, 13),
+        multiplyAndRoundUp(createResource(3, 1, 5), 2.5));
+    assertEquals(INVALID_RESOURCE_MSG, createResource(8, 3, 0),
+        multiplyAndRoundUp(createResource(3, 1, 0), 2.5));
+    assertEquals(INVALID_RESOURCE_MSG, createResource(8, 8, 0),
+        multiplyAndRoundUp(createResource(3, 3, 0), 2.5));
+  }
+
   @Test
   public void testMultiplyAndRoundDown() {
-    assertEquals(createResource(4, 1),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(4, 1),
         multiplyAndRoundDown(createResource(3, 1), 1.5));
-    assertEquals(createResource(4, 1, 0),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(4, 1, 0),
         multiplyAndRoundDown(createResource(3, 1), 1.5));
-    assertEquals(createResource(1, 4),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(1, 4),
         multiplyAndRoundDown(createResource(1, 3), 1.5));
-    assertEquals(createResource(1, 4, 0),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(1, 4, 0),
         multiplyAndRoundDown(createResource(1, 3), 1.5));
-    assertEquals(createResource(7, 7, 0),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(7, 7, 0),
         multiplyAndRoundDown(createResource(3, 3, 0), 2.5));
-    assertEquals(createResource(2, 2, 7),
+    assertEquals(INVALID_RESOURCE_MSG, createResource(2, 2, 7),
         multiplyAndRoundDown(createResource(1, 1, 3), 2.5));
   }