Remove notion of node-specific reliability from Java distribution code

Not used, and wasn't algorithmically in sync with the C++ code anyway.
Also add guard to avoid emitting invalid node indices for storage nodes
if the number of configured nodes is lower than the replication factor.
Looks like this particular code path is only called by cross-language
conformance tests, so hasn't been a problem in practice.

4 files changed, 24 insertions, 62 deletions

diff --git a/vdslib/src/main/java/com/yahoo/vdslib/distribution/Distribution.java b/vdslib/src/main/java/com/yahoo/vdslib/distribution/Distribution.java
index accf3942e4d..f926b763711 100644
--- a/vdslib/src/main/java/com/yahoo/vdslib/distribution/Distribution.java
+++ b/vdslib/src/main/java/com/yahoo/vdslib/distribution/Distribution.java
@@ -215,11 +215,19 @@ public class Distribution {
     }
 
     private static class ScoredNode {
-        int index;
-        int reliability;
-        double score;
+        final double score;
+        final int index;
+
+        ScoredNode(int index, double score) {
+            this.score = score;
+            this.index = index;
+        }
 
-        ScoredNode(int index, int reliability, double score) { this.index = index; this.reliability = reliability; this.score = score; }
+        boolean valid() { return index != -1; }
+
+        static ScoredNode makeInvalid() {
+            return new ScoredNode(-1, 0.0);
+        }
     }
 
     private static boolean allDistributorsDown(Group g, ClusterState clusterState) {
@@ -409,7 +417,7 @@ public class Distribution {
             // avoid needing to check size during iteration.
             LinkedList<ScoredNode> tmpResults = new LinkedList<>();
             for (int i = 0; i < redundancy; ++i) {
-                tmpResults.add(new ScoredNode(0, 0, 0.0));
+                tmpResults.add(ScoredNode.makeInvalid());
             }
 
             for (ConfiguredNode configuredNode : nodes) {
@@ -449,7 +457,7 @@ public class Distribution {
                 if (score > tmpResults.getLast().score) {
                     for (int i = 0; i < tmpResults.size(); ++i) {
                         if (score > tmpResults.get(i).score) {
-                            tmpResults.add(i, new ScoredNode(configuredNode.index(), nodeState.getReliability(), score));
+                            tmpResults.add(i, new ScoredNode(configuredNode.index(), score));
                             break;
                         }
                     }
@@ -458,7 +466,9 @@ public class Distribution {
             }
 
             for (ScoredNode node : tmpResults) {
-                resultNodes.add(node.index);
+                if (node.valid()) {
+                    resultNodes.add(node.index);
+                }
             }
         }
 
@@ -492,7 +502,7 @@ public class Distribution {
         RandomGen random = new RandomGen(seed);
         int randomIndex = 0;
         List<ConfiguredNode> configuredNodes = idealGroup.getNodes();
-        ScoredNode node = new ScoredNode(0, 0, 0);
+        ScoredNode node = ScoredNode.makeInvalid();
         for (ConfiguredNode configuredNode : configuredNodes) {
             NodeState nodeState = state.getNodeState(new Node(NodeType.DISTRIBUTOR, configuredNode.index()));
             if (!nodeState.getState().oneOf(upStates)) continue;
@@ -512,10 +522,10 @@ public class Distribution {
                 score = Math.pow(score, 1.0 / nodeState.getCapacity());
             }
             if (score > node.score) {
-                node = new ScoredNode(configuredNode.index(), 1, score);
+                node = new ScoredNode(configuredNode.index(), score);
             }
         }
-        if (node.reliability == 0) {
+        if (!node.valid()) {
             throw new NoDistributorsAvailableException(
                     "No available distributors in any of the given upstates '"
                     + upStates + "'.");
diff --git a/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java b/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java
index 28913d94560..373e3d0a4ba 100644
--- a/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java
+++ b/vdslib/src/main/java/com/yahoo/vdslib/state/NodeState.java
@@ -77,7 +77,6 @@ public class NodeState implements Cloneable {
         NodeState ns = (NodeState) o;
         if (state != ns.state
             || Math.abs(capacity - ns.capacity)         > 0.0000000001
-            || Math.abs(reliability - ns.reliability)   > 0.0000000001
             || Math.abs(initProgress - ns.initProgress) > 0.0000000001
             || startTimestamp != ns.startTimestamp
             || minUsedBits != ns.minUsedBits)
@@ -105,7 +104,7 @@ public class NodeState implements Cloneable {
         return true;
     }
     public int hashCode() {
-        return state.hashCode() ^ diskStates.hashCode() ^ Double.valueOf(capacity).hashCode() ^ Double.valueOf(reliability).hashCode();
+        return state.hashCode() ^ diskStates.hashCode() ^ Double.valueOf(capacity).hashCode();
     }
 
     /**
@@ -127,7 +126,6 @@ public class NodeState implements Cloneable {
     private boolean similarToImpl(final NodeState other, boolean considerInitProgress) {
         if (state != other.state) return false;
         if (Math.abs(capacity - other.capacity) > 0.0000000001) return false;
-        if (Math.abs(reliability - other.reliability) > 0.0000000001) return false;
         if (startTimestamp != other.startTimestamp) return false;
 
         // Init progress on different sides of the init progress limit boundary is not similar.
@@ -168,9 +166,6 @@ public class NodeState implements Cloneable {
         if (Math.abs(capacity - other.capacity) > 0.000000001) {
             diff.add(new Diff.Entry("capacity", capacity, other.capacity));
         }
-        if (Math.abs(reliability - other.reliability) > 0.000000001) {
-            diff.add(new Diff.Entry("reliability", reliability, other.reliability));
-        }
         if (minUsedBits != other.minUsedBits) {
             diff.add(new Diff.Entry("minUsedBits", minUsedBits, other.minUsedBits));
         }
@@ -206,7 +201,6 @@ public class NodeState implements Cloneable {
     /** Capacity is set by deserializing a node state. This seems odd, as it is config */
     public NodeState setCapacity(double c) { this.capacity = c; return this; }
 
-    public NodeState setReliability(int r) { this.reliability = r; return this; }
     public NodeState setInitProgress(double p) { this.initProgress = p; return this; }
     public NodeState setDescription(String desc) { this.description = desc; return this; }
     public NodeState setMinUsedBits(int u) { this.minUsedBits = u; return this; }
@@ -214,7 +208,6 @@ public class NodeState implements Cloneable {
     public NodeState setStartTimestamp(long ts) { this.startTimestamp = ts; return this; }
 
     public double getCapacity() { return this.capacity; }
-    public int getReliability() { return this.reliability; }
     public double getInitProgress() { return this.initProgress; }
     public boolean hasDescription() { return (description.length() > 0); }
     public String getDescription() { return description; }
@@ -238,9 +231,6 @@ public class NodeState implements Cloneable {
         if (Math.abs(capacity - 1.0) > 0.000000001) {
             sb.append(compact ? ", c " : ", capacity ").append(compact ? String.format(Locale.ENGLISH, "%.3g", capacity) : capacity);
         }
-        if (Math.abs(reliability - 1.0) > 0.000000001) {
-            sb.append(compact ? ", r " : ", reliability ").append(reliability);
-        }
         if (state.equals(State.INITIALIZING)) {
             sb.append(compact ? ", i " : ", init progress ").append(compact ? String.format(Locale.ENGLISH, "%.3g", initProgress) : initProgress);
             if (type.equals(NodeType.STORAGE)) {
@@ -324,10 +314,6 @@ public class NodeState implements Cloneable {
             if (empty) { empty = false; } else { sb.append(' '); }
             sb.append(prefix).append("c:").append(capacity);
         }
-        if (Math.abs(reliability - 1.0) > 0.000000001) {
-            if (empty) { empty = false; } else { sb.append(' '); }
-            sb.append(prefix).append("r:").append(reliability);
-        }
         if (state == State.INITIALIZING) {
             sb.append(' ');
             sb.append(prefix).append("i:").append(initProgress);
@@ -416,15 +402,6 @@ public class NodeState implements Cloneable {
                     throw new ParseException("Illegal capacity '" + value + "'. Capacity must be a positive floating point number", 0);
                 }
                 continue;
-            case 'r':
-                if (key.length() > 1) break;
-                if (type != null && !type.equals(NodeType.STORAGE)) break;
-                try{
-                    newState.setReliability(Integer.valueOf(value));
-                } catch (Exception e) {
-                    throw new ParseException("Illegal reliability '" + value + "'. Reliability must be a positive integer number", 0);
-                }
-                continue;
             case 'i':
                 if (key.length() > 1) break;
                 try{
@@ -500,9 +477,6 @@ public class NodeState implements Cloneable {
         if (type.equals(NodeType.DISTRIBUTOR) && Math.abs(capacity - 1.0) > 0.000000001) {
             throw new IllegalArgumentException("Capacity should not be set for a distributor node");
         }
-        if (type.equals(NodeType.DISTRIBUTOR) && Math.abs(reliability - 1.0) > 0.000000001) {
-            throw new IllegalArgumentException("Reliability should not be set for a distributor node");
-        }
         if (type.equals(NodeType.DISTRIBUTOR) && !diskStates.isEmpty()) {
             throw new IllegalArgumentException("Disk states should not be set for a distributor node");
         }
diff --git a/vdslib/src/test/java/com/yahoo/vdslib/state/ClusterStateTestCase.java b/vdslib/src/test/java/com/yahoo/vdslib/state/ClusterStateTestCase.java
index b622b596bb7..9a8413b250a 100644
--- a/vdslib/src/test/java/com/yahoo/vdslib/state/ClusterStateTestCase.java
+++ b/vdslib/src/test/java/com/yahoo/vdslib/state/ClusterStateTestCase.java
@@ -34,7 +34,7 @@ public class ClusterStateTestCase{
     public void testClone() throws ParseException {
         ClusterState state = new ClusterState("");
         state.setNodeState(new Node(NodeType.DISTRIBUTOR, 1), new NodeState(NodeType.DISTRIBUTOR, State.UP).setDescription("available"));
-        state.setNodeState(new Node(NodeType.STORAGE, 0), new NodeState(NodeType.STORAGE, State.UP).setCapacity(1.2).setReliability(2));
+        state.setNodeState(new Node(NodeType.STORAGE, 0), new NodeState(NodeType.STORAGE, State.UP).setCapacity(1.2));
         state.setNodeState(new Node(NodeType.STORAGE, 2), new NodeState(NodeType.STORAGE, State.UP).setDiskCount(2).setDiskState(1, new DiskState(State.DOWN)));
         ClusterState other = state.clone();
         assertEquals(state.toString(true), other.toString(true));
diff --git a/vdslib/src/test/java/com/yahoo/vdslib/state/NodeStateTestCase.java b/vdslib/src/test/java/com/yahoo/vdslib/state/NodeStateTestCase.java
index bbd9f8b6d56..dbc5ad59edf 100644
--- a/vdslib/src/test/java/com/yahoo/vdslib/state/NodeStateTestCase.java
+++ b/vdslib/src/test/java/com/yahoo/vdslib/state/NodeStateTestCase.java
@@ -42,20 +42,6 @@ public class NodeStateTestCase {
     }
 
     @Test
-    public void testTrivialitiesToIncreaseCoverage() throws ParseException {
-        NodeState ns = new NodeState(NodeType.STORAGE, State.UP);
-        assertEquals(1, ns.getReliability());
-        assertEquals(false, ns.isAnyDiskDown());
-
-        assertEquals(ns.setReliability(2).serialize(), NodeState.deserialize(NodeType.STORAGE, "r:2").serialize());
-        assertEquals(ns.setDiskCount(1).serialize(), NodeState.deserialize(NodeType.STORAGE, "r:2 d:1").serialize());
-        assertEquals(ns.setReliability(1).serialize(), NodeState.deserialize(NodeType.STORAGE, "d:1").serialize());
-
-        assertEquals(ns.setDiskState(0, new DiskState(State.DOWN, "", 1)), NodeState.deserialize(NodeType.STORAGE, "s:u d:1 d.0.s:d"));
-        assertEquals(ns, NodeState.deserialize(NodeType.STORAGE, "s:u d:1 d.0:d"));
-    }
-
-    @Test
     public void testDiskState() throws ParseException {
         NodeState ns = NodeState.deserialize(NodeType.STORAGE, "s:m");
         assertEquals(new DiskState(State.UP, "", 1), ns.getDiskState(0));
@@ -113,10 +99,6 @@ public class NodeStateTestCase {
             assertTrue("Should fail", false);
         } catch (Exception e) {}
         try {
-            NodeState.deserialize(NodeType.STORAGE, "s:m r:badvalue");
-            assertTrue("Should fail", false);
-        } catch (Exception e) {}
-        try {
             NodeState.deserialize(NodeType.STORAGE, "s:m i:badvalue");
             assertTrue("Should fail", false);
         } catch (Exception e) {}
@@ -222,12 +204,12 @@ public class NodeStateTestCase {
         String expected = "Maintenance => Up";
         assertEquals(expected, ns.getTextualDifference(new NodeState(NodeType.STORAGE, State.UP)).substring(0, expected.length()));
 
-        NodeState ns1 = new NodeState(NodeType.STORAGE, State.MAINTENANCE).setDescription("Foo bar").setCapacity(1.2).setReliability(2).setDiskCount(4)
+        NodeState ns1 = new NodeState(NodeType.STORAGE, State.MAINTENANCE).setDescription("Foo bar").setCapacity(1.2).setDiskCount(4)
                 .setMinUsedBits(12).setStartTimestamp(5).setDiskState(1, new DiskState(State.DOWN, "bad disk", 1))
                 .setDiskState(3, new DiskState(State.UP, "", 2));
         ns1.toString();
         ns1.toString(true);
-        expected = "Maintenance => Up, capacity: 1.2 => 1.0, reliability: 2 => 1, minUsedBits: 12 => 16, startTimestamp: 5 => 0, disks: 4 => 0, description: Foo bar => ";
+        expected = "Maintenance => Up, capacity: 1.2 => 1.0, minUsedBits: 12 => 16, startTimestamp: 5 => 0, disks: 4 => 0, description: Foo bar => ";
         assertEquals(expected, ns1.getTextualDifference(new NodeState(NodeType.STORAGE, State.UP)).substring(0, expected.length()));
     }
 
@@ -242,10 +224,6 @@ public class NodeStateTestCase {
             assertTrue("Should not be valid", false);
         } catch (Exception e) {}
         try{
-            new NodeState(NodeType.DISTRIBUTOR, State.UP).setReliability(3).verifyValidInSystemState(NodeType.DISTRIBUTOR);
-            assertTrue("Should not be valid", false);
-        } catch (Exception e) {}
-        try{
             new NodeState(NodeType.DISTRIBUTOR, State.UP).setDiskCount(2).verifyValidInSystemState(NodeType.DISTRIBUTOR);
             assertTrue("Should not be valid", false);
         } catch (Exception e) {}