Code review: Renamed methods, variables, added class description

4 files changed, 42 insertions, 33 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareChecker.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareChecker.java
index c4692e01cc9..aa9a1fb931d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareChecker.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareChecker.java
@@ -10,12 +10,21 @@ import java.util.Map;
 import java.util.Set;
 
 /**
+ * This class helps answer the question if there are enough nodes to retire a node with flavor f by:
+ * <ul>
+ *   <li>Finding all the possible flavors that the replacement node could end up on</li>
+ *   <li>Making sure that regardless of which flavor it ends up on, there is still enough spare nodes
+ *       to handle at unexpected node failures.</li>
+ * </ul>
+ * <p>
  * Definitions:
- * - Wanted flavor: The flavor that is the node prefers, for example by specifying in services.xml
- * - Node-repo flavor: The flavor that the node actually has (Either the wanted flavor or a flavor that transitively
- *                     replaces the wanted flavor)
- * - Replacee flavor: Flavor x is replacee of y iff x transitively replaces y
- * - Immediate replacee flavor: Flavor x is an immediate replacee of flavor y iff x directly replaces y.
+ * <ul>
+ *   <li>Wanted flavor: The flavor that is the node prefers, for example by specifying in services.xml</li>
+ *   <li>Node-repo flavor: The flavor that the node actually has (Either the wanted flavor or a flavor that transitively
+ *                     replaces the wanted flavor)</li>
+ *   <li>Replacee flavor: Flavor x is replacee of y iff x transitively replaces y</li>
+ *   <li>Immediate replacee flavor: Flavor x is an immediate replacee of flavor y iff x directly replaces y.</li>
+ * </ul>
  *
  * @author freva
  */
@@ -39,14 +48,14 @@ public class FlavorSpareChecker {
 
     public boolean canRetireAllocatedNodeWithFlavor(Flavor flavor) {
         Set<FlavorSpareCount> possibleNewFlavors = findPossibleReplacementFlavorFor(spareCountByFlavor.get(flavor));
-        possibleNewFlavors.forEach(FlavorSpareCount::decrementNumberOfSpares);
+        possibleNewFlavors.forEach(FlavorSpareCount::decrementNumberOfReady);
         return !possibleNewFlavors.isEmpty();
     }
 
     public boolean canRetireUnallocatedNodeWithFlavor(Flavor flavor) {
         FlavorSpareCount flavorSpareCount = spareCountByFlavor.get(flavor);
         if (flavorSpareCount.hasReady() && spareNodesPolicy.hasSpare(flavorSpareCount)) {
-            flavorSpareCount.decrementNumberOfSpares();
+            flavorSpareCount.decrementNumberOfReady();
             return true;
         }
 
@@ -61,44 +70,44 @@ public class FlavorSpareChecker {
      * The algorithm is:
      * for all possible wanted flavor, check:
      * <ul>
-     *   <li>1: Sum of spare nodes of flavor f and all replacee flavors of f is &gt; 0</li>
+     *   <li>1: Sum of ready nodes of flavor f and all replacee flavors of f is &gt; reserved (set by {@link SpareNodesPolicy}</li>
      *   <li>2a: Number of ready nodes of flavor f is &gt; 0</li>
-     *   <li>2b: Verify 1 &amp; 2a for all immediate replacee of f, f_i, where sum of ready nodes of f_i and all
+     *   <li>2b: Verify 1 &amp; 2 for all immediate replacee of f, f_i, where sum of ready nodes of f_i and all
      *       replacee flavors of f_i is &gt; 0</li>
  *     </ul>
      * Only 2a OR 2b need to be satisfied.
      */
     private Set<FlavorSpareCount> findPossibleReplacementFlavorFor(FlavorSpareCount flavorSpareCount) {
-        Set<FlavorSpareCount> possibleNewFlavors = new HashSet<>();
+        Set<FlavorSpareCount> possibleReplacementFlavors = new HashSet<>();
         for (FlavorSpareCount possibleWantedFlavor : flavorSpareCount.getPossibleWantedFlavors()) {
-            Set<FlavorSpareCount> newFlavors = verifyReplacementConditions(possibleWantedFlavor);
-            if (newFlavors.isEmpty()) return Collections.emptySet();
-            else possibleNewFlavors.addAll(newFlavors);
+            Set<FlavorSpareCount> replacementFlavors = verifyReplacementConditions(possibleWantedFlavor);
+            if (replacementFlavors.isEmpty()) return Collections.emptySet();
+            else possibleReplacementFlavors.addAll(replacementFlavors);
         }
 
-        return possibleNewFlavors;
+        return possibleReplacementFlavors;
     }
 
     private Set<FlavorSpareCount> verifyReplacementConditions(FlavorSpareCount flavorSpareCount) {
-        Set<FlavorSpareCount> possibleNewFlavors = new HashSet<>();
+        Set<FlavorSpareCount> possibleReplacementFlavors = new HashSet<>();
         // Breaks condition 1, end
         if (! spareNodesPolicy.hasSpare(flavorSpareCount)) return Collections.emptySet();
 
         // Condition 2a
         if (flavorSpareCount.hasReady()) {
-            possibleNewFlavors.add(flavorSpareCount);
+            possibleReplacementFlavors.add(flavorSpareCount);
 
         // Condition 2b
         } else {
             for (FlavorSpareCount possibleNewFlavor : flavorSpareCount.getImmediateReplacees()) {
-                if (possibleNewFlavor.getSumOfReadyAmongReplacees() == 0) continue;
+                if (possibleNewFlavor.getNumReadyAmongReplacees() == 0) continue;
 
-                Set<FlavorSpareCount> newFlavors = verifyReplacementConditions(possibleNewFlavor);
-                if (newFlavors.isEmpty()) return Collections.emptySet();
-                else possibleNewFlavors.addAll(newFlavors);
+                Set<FlavorSpareCount> replacementFlavors = verifyReplacementConditions(possibleNewFlavor);
+                if (replacementFlavors.isEmpty()) return Collections.emptySet();
+                else possibleReplacementFlavors.addAll(replacementFlavors);
             }
         }
-        return possibleNewFlavors;
+        return possibleReplacementFlavors;
     }
 
     public interface SpareNodesPolicy {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCount.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCount.java
index 2acb7adb804..1ff71485434 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCount.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCount.java
@@ -74,10 +74,10 @@ public class FlavorSpareCount {
         return numReady > 0;
     }
 
-    public long getSumOfReadyAmongReplacees() {
+    public long getNumReadyAmongReplacees() {
         long sumReadyNodes = numReady;
         for (FlavorSpareCount replacee : immediateReplacees) {
-            sumReadyNodes += replacee.getSumOfReadyAmongReplacees();
+            sumReadyNodes += replacee.getNumReadyAmongReplacees();
         }
 
         return sumReadyNodes;
@@ -91,7 +91,7 @@ public class FlavorSpareCount {
         return immediateReplacees;
     }
 
-    void decrementNumberOfSpares() {
+    void decrementNumberOfReady() {
         numReady--;
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCheckerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCheckerTest.java
index f41f2983cd4..b8947635f93 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCheckerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCheckerTest.java
@@ -154,8 +154,8 @@ public class FlavorSpareCheckerTest {
                     when(spareNodesPolicy.hasSpare(flavorSpareCount)).thenReturn(true);
                 });
         when(flavorSpareCountByFlavor.get(flavorToRetire).hasReady()).thenReturn(false);
-        when(flavorSpareCountByFlavor.get(flavors.get(0)).getSumOfReadyAmongReplacees()).thenReturn(1L);
-        when(flavorSpareCountByFlavor.get(flavors.get(2)).getSumOfReadyAmongReplacees()).thenReturn(1L);
+        when(flavorSpareCountByFlavor.get(flavors.get(0)).getNumReadyAmongReplacees()).thenReturn(1L);
+        when(flavorSpareCountByFlavor.get(flavors.get(2)).getNumReadyAmongReplacees()).thenReturn(1L);
 
         assertTrue(flavorSpareChecker.canRetireAllocatedNodeWithFlavor(flavorToRetire));
         verifyDecrement(0, 2, 3, 4, 5);
@@ -174,8 +174,8 @@ public class FlavorSpareCheckerTest {
                     when(spareNodesPolicy.hasSpare(flavorSpareCount)).thenReturn(true);
                 });
         when(flavorSpareCountByFlavor.get(flavorToRetire).hasReady()).thenReturn(false);
-        when(flavorSpareCountByFlavor.get(flavors.get(0)).getSumOfReadyAmongReplacees()).thenReturn(1L);
-        when(flavorSpareCountByFlavor.get(flavors.get(2)).getSumOfReadyAmongReplacees()).thenReturn(1L);
+        when(flavorSpareCountByFlavor.get(flavors.get(0)).getNumReadyAmongReplacees()).thenReturn(1L);
+        when(flavorSpareCountByFlavor.get(flavors.get(2)).getNumReadyAmongReplacees()).thenReturn(1L);
 
         assertFalse(flavorSpareChecker.canRetireAllocatedNodeWithFlavor(flavorToRetire));
         verifyDecrement();
@@ -195,8 +195,8 @@ public class FlavorSpareCheckerTest {
                     when(spareNodesPolicy.hasSpare(flavorSpareCount)).thenReturn(true);
                 });
         when(flavorSpareCountByFlavor.get(flavorToRetire).hasReady()).thenReturn(false);
-        when(flavorSpareCountByFlavor.get(flavors.get(0)).getSumOfReadyAmongReplacees()).thenReturn(1L);
-        when(flavorSpareCountByFlavor.get(flavors.get(2)).getSumOfReadyAmongReplacees()).thenReturn(0L);
+        when(flavorSpareCountByFlavor.get(flavors.get(0)).getNumReadyAmongReplacees()).thenReturn(1L);
+        when(flavorSpareCountByFlavor.get(flavors.get(2)).getNumReadyAmongReplacees()).thenReturn(0L);
 
         assertTrue(flavorSpareChecker.canRetireAllocatedNodeWithFlavor(flavorToRetire));
         verifyDecrement(0, 3, 4, 5);
@@ -206,7 +206,7 @@ public class FlavorSpareCheckerTest {
         Set<Flavor> decrementedFlavors = Arrays.stream(decrementFlavorIds).boxed().map(flavors::get).collect(Collectors.toSet());
         for (Flavor flavor : flavors) {
             int times = decrementedFlavors.contains(flavor) ? 1 : 0;
-            verify(flavorSpareCountByFlavor.get(flavor), times(times)).decrementNumberOfSpares();
+            verify(flavorSpareCountByFlavor.get(flavor), times(times)).decrementNumberOfReady();
         }
     }
 
@@ -219,7 +219,7 @@ public class FlavorSpareCheckerTest {
             Set<FlavorSpareCount> immediateReplacees = flavorSpareCountGraph.get(flavor).getImmediateReplacees()
                     .stream().map(FlavorSpareCount::getFlavor).map(flavorSpareCountByFlavor::get).collect(Collectors.toSet());
 
-            doNothing().when(flavorSpareCount).decrementNumberOfSpares();
+            doNothing().when(flavorSpareCount).decrementNumberOfReady();
             when(flavorSpareCount.hasReady()).thenReturn(false);
             when(flavorSpareCount.getPossibleWantedFlavors()).thenReturn(possibleWantedFlavors);
             when(flavorSpareCount.getImmediateReplacees()).thenReturn(immediateReplacees);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCountTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCountTest.java
index 6de8a1391c0..5f20750517c 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCountTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorSpareCountTest.java
@@ -93,7 +93,7 @@ public class FlavorSpareCountTest {
 
         long[] expectedSumTrees = {3, 10, 2, 16, 16, 23, 4, 3, 7};
         for (int i = 0; i < expectedSumTrees.length; i++) {
-            assertEquals(expectedSumTrees[i], flavorSpareCountByFlavor.get(flavors.get(i)).getSumOfReadyAmongReplacees());
+            assertEquals(expectedSumTrees[i], flavorSpareCountByFlavor.get(flavors.get(i)).getNumReadyAmongReplacees());
         }
     }