Remove notion of node-specific reliability from C++ distribution code

I have never seen this in use anywhere, and can find no code that ever
sets it. Bonus is that node candidate trimming can be vastly simplified.

5 files changed, 32 insertions, 119 deletions

diff --git a/vdslib/src/tests/state/clusterstatetest.cpp b/vdslib/src/tests/state/clusterstatetest.cpp
index 1880683232d..2dda26f8882 100644
--- a/vdslib/src/tests/state/clusterstatetest.cpp
+++ b/vdslib/src/tests/state/clusterstatetest.cpp
@@ -87,8 +87,8 @@ TEST(ClusterStateTest, test_basic_functionality)
     // Test other storage node propertise
     // (Messages is excluded from system states to not make them too long as
     // most nodes have no use for them)
-    VERIFYNEW("storage:9 .3.c:2.3 .4.r:8 .7.m:foo\\x20bar",
-              "storage:9 .3.c:2.3 .4.r:8");
+    VERIFYNEW("storage:9 .3.c:2.3 .7.m:foo\\x20bar",
+              "storage:9 .3.c:2.3");
 
     // Test that messages are kept in verbose mode, even if last index
     {
@@ -159,7 +159,7 @@ TEST(ClusterStateTest, test_detailed)
     ClusterState state(
             "version:314 cluster:i "
             "distributor:8 .1.s:i .3.s:i .3.i:0.5 .5.s:d .7.m:foo\\x20bar "
-            "storage:10 .2.d:16 .2.d.3:d .4.s:d .5.c:1.3 .5.r:4"
+            "storage:10 .2.d:16 .2.d.3:d .4.s:d .5.c:1.3 "
             " .6.m:bar\\tfoo .7.s:m .8.d:10 .8.d.4.c:0.6 .8.d.4.m:small"
     );
     EXPECT_EQ(314u, state.getVersion());
@@ -170,7 +170,7 @@ TEST(ClusterStateTest, test_detailed)
     // Testing distributor node states
     for (uint16_t i = 0; i <= 20; ++i) {
         const NodeState& ns(state.getNodeState(Node(NodeType::DISTRIBUTOR, i)));
-            // Test node states
+        // Test node states
         if (i == 1 || i == 3) {
             EXPECT_EQ(State::INITIALIZING, ns.getState());
         } else if (i == 5 || i >= 8) {
@@ -178,7 +178,7 @@ TEST(ClusterStateTest, test_detailed)
         } else {
             EXPECT_EQ(State::UP, ns.getState());
         }
-            // Test initialize progress
+        // Test initialize progress
         if (i == 1) {
             EXPECT_EQ(vespalib::Double(0.0), ns.getInitProgress());
         } else if (i == 3) {
@@ -186,7 +186,7 @@ TEST(ClusterStateTest, test_detailed)
         } else {
             EXPECT_EQ(vespalib::Double(0.0), ns.getInitProgress());
         }
-            // Test message
+        // Test message
         if (i == 7) {
             EXPECT_EQ(string("foo bar"), ns.getDescription());
         } else {
@@ -197,7 +197,7 @@ TEST(ClusterStateTest, test_detailed)
     // Testing storage node states
     for (uint16_t i = 0; i <= 20; ++i) {
         const NodeState& ns(state.getNodeState(Node(NodeType::STORAGE, i)));
-            // Test node states
+        // Test node states
         if (i == 4 || i >= 10) {
             EXPECT_EQ(State::DOWN, ns.getState());
         } else if (i == 7) {
@@ -211,13 +211,7 @@ TEST(ClusterStateTest, test_detailed)
         } else {
             EXPECT_EQ(string(""), ns.getDescription());
         }
-            // Test reliability
-        if (i == 5) {
-            EXPECT_EQ(uint16_t(4), ns.getReliability());
-        } else {
-            EXPECT_EQ(uint16_t(1), ns.getReliability());
-        }
-            // Test capacity
+        // Test capacity
         if (i == 5) {
             EXPECT_EQ(vespalib::Double(1.3), ns.getCapacity());
         } else {
diff --git a/vdslib/src/tests/state/nodestatetest.cpp b/vdslib/src/tests/state/nodestatetest.cpp
index da6faa49779..370c20eef8d 100644
--- a/vdslib/src/tests/state/nodestatetest.cpp
+++ b/vdslib/src/tests/state/nodestatetest.cpp
@@ -11,13 +11,11 @@ TEST(NodeStateTest, test_parsing)
         NodeState ns = NodeState("s:u");
         EXPECT_EQ(std::string("s:u"), ns.toString());
         EXPECT_EQ(vespalib::Double(1.0), ns.getCapacity());
-        EXPECT_EQ(uint16_t(1), ns.getReliability());
     }
     {
         NodeState ns = NodeState("s:m");
         EXPECT_EQ(std::string("s:m"), ns.toString());
         EXPECT_EQ(vespalib::Double(1.0), ns.getCapacity());
-        EXPECT_EQ(uint16_t(1), ns.getReliability());
     }
     {
         NodeState ns = NodeState("t:4");
@@ -25,31 +23,27 @@ TEST(NodeStateTest, test_parsing)
         EXPECT_EQ(uint64_t(4), ns.getStartTimestamp());
     }
     {
-        NodeState ns = NodeState("s:u c:2.4 r:3 b:12");
-        EXPECT_EQ(std::string("s:u c:2.4 r:3 b:12"), ns.toString());
+        NodeState ns = NodeState("s:u c:2.4 b:12");
+        EXPECT_EQ(std::string("s:u c:2.4 b:12"), ns.toString());
         EXPECT_EQ(vespalib::Double(2.4), ns.getCapacity());
-        EXPECT_EQ(uint16_t(3), ns.getReliability());
         EXPECT_EQ(12, (int)ns.getMinUsedBits());
 
         EXPECT_NE(NodeState("s:u b:12"), NodeState("s:u b:13"));
     }
     {
-        NodeState ns = NodeState("c:2.4\ns:u\nr:5");
-        EXPECT_EQ(std::string("s:u c:2.4 r:5"), ns.toString());
+        NodeState ns = NodeState("c:2.4\ns:u");
+        EXPECT_EQ(std::string("s:u c:2.4"), ns.toString());
         EXPECT_EQ(vespalib::Double(2.4), ns.getCapacity());
-        EXPECT_EQ(uint16_t(5), ns.getReliability());
     }
     {
-        NodeState ns = NodeState("c:2.4 r:1");
+        NodeState ns = NodeState("c:2.4");
         EXPECT_EQ(std::string("s:u c:2.4"), ns.toString());
         EXPECT_EQ(vespalib::Double(2.4), ns.getCapacity());
-        EXPECT_EQ(uint16_t(1), ns.getReliability());
     }
     {
         NodeState ns = NodeState("c:2.4 k:2.6");
         EXPECT_EQ(std::string("s:u c:2.4"), ns.toString());
         EXPECT_EQ(vespalib::Double(2.4), ns.getCapacity());
-        EXPECT_EQ(uint16_t(1), ns.getReliability());
     }
 }
 
diff --git a/vdslib/src/vespa/vdslib/distribution/distribution.cpp b/vdslib/src/vespa/vdslib/distribution/distribution.cpp
index d51e9ee4f49..c5b9ec855d9 100644
--- a/vdslib/src/vespa/vdslib/distribution/distribution.cpp
+++ b/vdslib/src/vespa/vdslib/distribution/distribution.cpp
@@ -225,50 +225,27 @@ namespace {
 
     /** Used to record scored nodes during ideal nodes calculation. */
     struct ScoredNode {
-        double _score;
+        double   _score;
         uint16_t _index;
-        uint16_t _reliability;
 
-        ScoredNode() noexcept : _score(0), _index(0), _reliability(0) {}
-        ScoredNode(double score, uint16_t index, uint16_t reliability) noexcept
-            : _score(score), _index(index), _reliability(reliability) {}
+        constexpr ScoredNode() noexcept : _score(0), _index(UINT16_MAX) {}
+        constexpr ScoredNode(double score, uint16_t index) noexcept
+            : _score(score), _index(index) {}
 
-        bool operator<(const ScoredNode& other) const noexcept {
+        constexpr bool operator<(const ScoredNode& other) const noexcept {
             return (_score < other._score);
         }
+        constexpr bool valid() const noexcept {
+            return (_index != UINT16_MAX);
+        }
     };
 
-    /**
-     * Throw away last entries until throwing away another would
-     * decrease redundancy below total reliability. If redundancy !=
-     * total reliability, see if non-last entries can be removed.
-     */
+    // Trim the input vector so that no trailing invalid entries remain and that
+    // it has a maximum size of `redundancy`.
     void
     trimResult(std::vector<ScoredNode>& nodes, uint16_t redundancy) {
-            // Initially record total reliability and use the first elements
-            // until satisfied.
-        uint32_t totalReliability = 0;
-        for (auto it = nodes.begin(); it != nodes.end(); ++it) {
-            if (totalReliability >= redundancy || it->_reliability == 0) {
-                nodes.erase(it, nodes.end());
-                break;
-            }
-            totalReliability += it->_reliability;
-        }
-            // If we have too high reliability, see if we can remove something
-            // else
-        if (totalReliability > redundancy) {
-            for (auto it = nodes.rbegin(); it != nodes.rend();) {
-                if (it->_reliability <= (totalReliability - redundancy)) {
-                    totalReliability -= it->_reliability;
-                    auto deleteIt(it.base());
-                    ++it;
-                    nodes.erase(--deleteIt);
-                    if (totalReliability == redundancy) break;
-                } else {
-                    ++it;
-                }
-            }
+        while (!nodes.empty() && (!nodes.back().valid() || (nodes.size() > redundancy))) {
+            nodes.pop_back();
         }
     }
 
@@ -456,7 +433,7 @@ Distribution::getIdealNodes(const NodeType& nodeType,
                 score = std::pow(score, 1.0 / nodeState.getCapacity().getValue());
             }
             if (score > tmpResults.back()._score) {
-                insertOrdered(tmpResults, ScoredNode(score, nodes[j], nodeState.getReliability()));
+                insertOrdered(tmpResults, ScoredNode(score, nodes[j]));
             }
         }
         trimResult(tmpResults, groupRedundancy);
diff --git a/vdslib/src/vespa/vdslib/state/nodestate.cpp b/vdslib/src/vespa/vdslib/state/nodestate.cpp
index fc4e18fa0cc..96ba4ac556f 100644
--- a/vdslib/src/vespa/vdslib/state/nodestate.cpp
+++ b/vdslib/src/vespa/vdslib/state/nodestate.cpp
@@ -26,7 +26,6 @@ NodeState::NodeState()
       _state(0),
       _description(""),
       _capacity(1.0),
-      _reliability(1),
       _initProgress(0.0),
       _minUsedBits(16),
       _startTimestamp(0)
@@ -35,13 +34,11 @@ NodeState::NodeState()
 }
 
 NodeState::NodeState(const NodeType& type, const State& state,
-                     vespalib::stringref description,
-                     double capacity, uint16_t reliability)
+                     vespalib::stringref description, double capacity)
     : _type(&type),
       _state(0),
       _description(description),
       _capacity(1.0),
-      _reliability(1),
       _initProgress(0.0),
       _minUsedBits(16),
       _startTimestamp(0)
@@ -49,7 +46,6 @@ NodeState::NodeState(const NodeType& type, const State& state,
     setState(state);
     if (type == NodeType::STORAGE) {
         setCapacity(capacity);
-        setReliability(reliability);
     }
 }
 
@@ -58,7 +54,6 @@ NodeState::NodeState(vespalib::stringref serialized, const NodeType* type)
       _state(&State::UP),
       _description(),
       _capacity(1.0),
-      _reliability(1),
       _initProgress(0.0),
       _minUsedBits(16),
       _startTimestamp(0)
@@ -105,18 +100,6 @@ NodeState::NodeState(vespalib::stringref serialized, const NodeType* type)
                             "a positive floating point number", VESPA_STRLOC);
                 }
                 continue;
-            case 'r':
-                if (_type != 0 && *type != NodeType::STORAGE) break;
-                if (key.size() > 1) break;
-                try{
-                    setReliability(boost::lexical_cast<uint16_t>(value));
-                } catch (...) {
-                    throw vespalib::IllegalArgumentException(
-                            "Illegal reliability '" + value + "'. Reliability "
-                            "must be a positive integer",
-                            VESPA_STRLOC);
-                }
-                continue;
             case 'i':
                 if (key.size() > 1) break;
                 try{
@@ -180,8 +163,8 @@ NodeState::serialize(vespalib::asciistream & out, vespalib::stringref prefix,
                      bool includeDescription) const
 {
     SeparatorPrinter sep;
-        // Always give node state if not part of a system state
-        // to prevent empty serialization
+    // Always give node state if not part of a system state
+    // to prevent empty serialization
     if (*_state != State::UP || prefix.size() == 0) {
         out << sep << prefix << "s:";
         out << _state->serialize();
@@ -189,9 +172,6 @@ NodeState::serialize(vespalib::asciistream & out, vespalib::stringref prefix,
     if (_capacity != 1.0) {
         out << sep << prefix << "c:" << _capacity;
     }
-    if (_reliability != 1) {
-        out << sep << prefix << "r:" << _reliability;
-    }
     if (_minUsedBits != 16) {
         out << sep << prefix << "b:" << _minUsedBits;
     }
@@ -211,8 +191,8 @@ void
 NodeState::setState(const State& state)
 {
     if (_type != 0) {
-            // We don't know whether you want to store reported, wanted or
-            // current node state, so we must accept any.
+        // We don't know whether you want to store reported, wanted or
+        // current node state, so we must accept any.
         if (!state.validReportedNodeState(*_type)
             && !state.validWantedNodeState(*_type))
         {
@@ -253,22 +233,6 @@ NodeState::setCapacity(vespalib::Double capacity)
 }
 
 void
-NodeState::setReliability(uint16_t reliability)
-{
-    if (reliability == 0) {
-        std::ostringstream ost;
-        ost << "Illegal reliability '" << reliability << "'. Reliability "
-                "must be a positive integer.";
-        throw vespalib::IllegalArgumentException(ost.str(), VESPA_STRLOC);
-    }
-    if (_type != 0 && *_type != NodeType::STORAGE) {
-        throw vespalib::IllegalArgumentException(
-                "Reliability only make sense for storage nodes.", VESPA_STRLOC);
-    }
-    _reliability = reliability;
-}
-
-void
 NodeState::setInitProgress(vespalib::Double initProgress)
 {
     if (initProgress < 0 || initProgress > 1.0) {
@@ -300,9 +264,6 @@ NodeState::print(std::ostream& out, bool verbose,
     if (_capacity != 1.0) {
         out << ", capacity " << _capacity;
     }
-    if (_reliability != 1) {
-        out << ", reliability " << _reliability;
-    }
     if (_minUsedBits != 16) {
         out << ", minimum used bits " << _minUsedBits;
     }
@@ -322,7 +283,6 @@ NodeState::operator==(const NodeState& other) const
 {
     if (_state != other._state ||
         _capacity != other._capacity ||
-        _reliability != other._reliability ||
         _minUsedBits != other._minUsedBits ||
         _startTimestamp != other._startTimestamp ||
         (*_state == State::INITIALIZING
@@ -338,7 +298,6 @@ NodeState::similarTo(const NodeState& other) const
 {
     if (_state != other._state ||
         _capacity != other._capacity ||
-        _reliability != other._reliability ||
         _minUsedBits != other._minUsedBits ||
         _startTimestamp < other._startTimestamp)
     {
@@ -370,10 +329,6 @@ NodeState::verifySupportForNodeType(const NodeType& type) const
         throw vespalib::IllegalArgumentException("Capacity should not be "
                 "set for a distributor node.", VESPA_STRLOC);
     }
-    if (type == NodeType::DISTRIBUTOR && _reliability != 1) {
-        throw vespalib::IllegalArgumentException("Reliability should not be "
-                "set for a distributor node.", VESPA_STRLOC);
-    }
 }
 
 std::string
@@ -389,10 +344,6 @@ NodeState::getTextualDifference(const NodeState& other) const {
         source << ", capacity " << _capacity;
         target << ", capacity " << other._capacity;
     }
-    if (_reliability != other._reliability) {
-        source << ", reliability " << _reliability;
-        target << ", reliability " << other._reliability;
-    }
     if (_minUsedBits != other._minUsedBits) {
         source << ", minUsedBits " << _minUsedBits;
         target << ", minUsedBits " << _minUsedBits;
diff --git a/vdslib/src/vespa/vdslib/state/nodestate.h b/vdslib/src/vespa/vdslib/state/nodestate.h
index 685030e948f..f0ae7012dcb 100644
--- a/vdslib/src/vespa/vdslib/state/nodestate.h
+++ b/vdslib/src/vespa/vdslib/state/nodestate.h
@@ -23,7 +23,6 @@ class NodeState : public document::Printable
     const State* _state;
     vespalib::string _description;
     vespalib::Double _capacity;
-    uint16_t _reliability;
     vespalib::Double _initProgress;
     uint32_t _minUsedBits;
     uint64_t _startTimestamp;
@@ -42,7 +41,7 @@ public:
     NodeState & operator = (NodeState &&) noexcept;
     NodeState(const NodeType& nodeType, const State&,
               vespalib::stringref description = "",
-              double capacity = 1.0, uint16_t reliability = 1);
+              double capacity = 1.0);
     /** Set type if you want to verify that content fit with the given type. */
     NodeState(vespalib::stringref serialized, const NodeType* nodeType = 0);
     ~NodeState();
@@ -58,7 +57,6 @@ public:
     const State& getState() const { return *_state; }
     vespalib::Double getCapacity() const { return _capacity; }
     uint32_t getMinUsedBits() const { return _minUsedBits; }
-    uint16_t getReliability() const { return _reliability; }
     vespalib::Double getInitProgress() const { return _initProgress; }
     const vespalib::string& getDescription() const { return _description; }
     uint64_t getStartTimestamp() const { return _startTimestamp; }
@@ -66,7 +64,6 @@ public:
     void setState(const State& state);
     void setCapacity(vespalib::Double capacity);
     void setMinUsedBits(uint32_t usedBits);
-    void setReliability(uint16_t reliability);
     void setInitProgress(vespalib::Double initProgress);
     void setStartTimestamp(uint64_t startTimestamp);
     void setDescription(vespalib::stringref desc) { _description = desc; }