Merge pull request #28390 from vespa-engine/balder/reduce-getNode-calls

Balder/reduce get node calls

18 files changed, 152 insertions, 186 deletions

diff --git a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
index 074b1492a6e..d3af4cd564a 100644
--- a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
+++ b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
@@ -20,7 +20,6 @@
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/text/stringtokenizer.h>
 #include <sstream>
-#include <iomanip>
 
 using namespace storage::api;
 using namespace storage::lib;
@@ -911,16 +910,11 @@ TEST_F(TopLevelBucketDBUpdaterTest, bit_change) {
                 for (int i=0; cnt < 2; i++) {
                     auto distribution = _component->getDistribution();
                     std::vector<uint16_t> distributors;
-                    if (distribution->getIdealDistributorNode(
-                            lib::ClusterState("bits:14 storage:1 distributor:2"),
-                            document::BucketId(16, i))
-                        == 0)
+                    if (distribution->getIdealDistributorNode(lib::ClusterState("bits:14 storage:1 distributor:2"),
+                                                              document::BucketId(16, i)) == 0)
                     {
-                        vec.push_back(api::RequestBucketInfoReply::Entry(
-                                document::BucketId(16, i),
-                                api::BucketInfo(10,1,1)));
-
-                        bucketlist.push_back(document::BucketId(16, i));
+                        vec.emplace_back(document::BucketId(16, i), api::BucketInfo(10,1,1));
+                        bucketlist.emplace_back(16, i);
                         cnt++;
                     }
                 }
@@ -953,14 +947,10 @@ TEST_F(TopLevelBucketDBUpdaterTest, bit_change) {
                 api::RequestBucketInfoReply::EntryVector &vec = sreply->getBucketInfo();
 
                 for (uint32_t i = 0; i < 3; ++i) {
-                    vec.push_back(api::RequestBucketInfoReply::Entry(
-                            document::BucketId(16, i),
-                            api::BucketInfo(10,1,1)));
+                    vec.emplace_back(document::BucketId(16, i), api::BucketInfo(10,1,1));
                 }
 
-                vec.push_back(api::RequestBucketInfoReply::Entry(
-                        document::BucketId(16, 4),
-                        api::BucketInfo(10,1,1)));
+                vec.emplace_back(document::BucketId(16, 4), api::BucketInfo(10,1,1));
             }
 
             bucket_db_updater().onRequestBucketInfoReply(sreply);
@@ -1047,9 +1037,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, recheck_node) {
     EXPECT_EQ(bucket.getBucketId(), rbi.getBuckets()[0]);
 
     auto reply = std::make_shared<api::RequestBucketInfoReply>(rbi);
-    reply->getBucketInfo().push_back(
-            api::RequestBucketInfoReply::Entry(document::BucketId(16, 3),
-                                               api::BucketInfo(20, 10, 12, 50, 60, true, true)));
+    reply->getBucketInfo().emplace_back(document::BucketId(16, 3), api::BucketInfo(20, 10, 12, 50, 60, true, true));
     stripe_bucket_db_updater.onRequestBucketInfoReply(reply);
 
     lib::ClusterState state("distributor:1 storage:3");
@@ -1110,8 +1098,8 @@ TEST_F(TopLevelBucketDBUpdaterTest, notify_bucket_change) {
     ASSERT_EQ(size_t(2), _sender.commands().size());
 
     std::vector<api::BucketInfo> infos;
-    infos.push_back(api::BucketInfo(4567, 200, 2000, 400, 4000, true, true));
-    infos.push_back(api::BucketInfo(8999, 300, 3000, 500, 5000, false, false));
+    infos.emplace_back(4567, 200, 2000, 400, 4000, true, true);
+    infos.emplace_back(8999, 300, 3000, 500, 5000, false, false);
 
     for (int i = 0; i < 2; ++i) {
         auto& rbi = dynamic_cast<RequestBucketInfoCommand&>(*_sender.command(i));
@@ -1120,7 +1108,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, notify_bucket_change) {
         EXPECT_EQ(bucket_id, rbi.getBuckets()[0]);
 
         auto reply = std::make_shared<api::RequestBucketInfoReply>(rbi);
-        reply->getBucketInfo().push_back(api::RequestBucketInfoReply::Entry(bucket_id, infos[i]));
+        reply->getBucketInfo().emplace_back(bucket_id, infos[i]);
         stripe_of_bucket(bucket_id).bucket_db_updater().onRequestBucketInfoReply(reply);
     }
 
@@ -1166,10 +1154,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, notify_bucket_change_from_node_down) {
     EXPECT_EQ(bucket_id, rbi.getBuckets()[0]);
 
     auto reply = std::make_shared<api::RequestBucketInfoReply>(rbi);
-    reply->getBucketInfo().push_back(
-            api::RequestBucketInfoReply::Entry(
-                    bucket_id,
-                    api::BucketInfo(8999, 300, 3000, 500, 5000, false, false)));
+    reply->getBucketInfo().emplace_back(bucket_id, api::BucketInfo(8999, 300, 3000, 500, 5000, false, false));
     stripe_of_bucket(bucket_id).bucket_db_updater().onRequestBucketInfoReply(reply);
 
     // No change
@@ -1233,9 +1218,9 @@ TEST_F(TopLevelBucketDBUpdaterTest, merge_reply) {
     add_nodes_to_stripe_bucket_db(bucket_id, "0=1234,1=1234,2=1234");
 
     std::vector<api::MergeBucketCommand::Node> nodes;
-    nodes.push_back(api::MergeBucketCommand::Node(0));
-    nodes.push_back(api::MergeBucketCommand::Node(1));
-    nodes.push_back(api::MergeBucketCommand::Node(2));
+    nodes.emplace_back(0);
+    nodes.emplace_back(1);
+    nodes.emplace_back(2);
 
     api::MergeBucketCommand cmd(makeDocumentBucket(bucket_id), nodes, 0);
     auto reply = std::make_shared<api::MergeBucketReply>(cmd);
@@ -1253,9 +1238,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, merge_reply) {
         EXPECT_EQ(bucket_id, req->getBuckets()[0]);
 
         auto reqreply = std::make_shared<api::RequestBucketInfoReply>(*req);
-        reqreply->getBucketInfo().push_back(
-                api::RequestBucketInfoReply::Entry(bucket_id,
-                                                   api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1))));
+        reqreply->getBucketInfo().emplace_back(bucket_id, api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1)));
 
         stripe_of_bucket(bucket_id).bucket_db_updater().onRequestBucketInfoReply(reqreply);
     }
@@ -1275,7 +1258,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, merge_reply_node_down) {
     add_nodes_to_stripe_bucket_db(bucket_id, "0=1234,1=1234,2=1234");
 
     for (uint32_t i = 0; i < 3; ++i) {
-        nodes.push_back(api::MergeBucketCommand::Node(i));
+        nodes.emplace_back(i);
     }
 
     api::MergeBucketCommand cmd(makeDocumentBucket(bucket_id), nodes, 0);
@@ -1296,10 +1279,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, merge_reply_node_down) {
         EXPECT_EQ(bucket_id, req->getBuckets()[0]);
 
         auto reqreply = std::make_shared<api::RequestBucketInfoReply>(*req);
-        reqreply->getBucketInfo().push_back(
-                api::RequestBucketInfoReply::Entry(
-                        bucket_id,
-                        api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1))));
+        reqreply->getBucketInfo().emplace_back(bucket_id, api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1)));
         stripe_of_bucket(bucket_id).bucket_db_updater().onRequestBucketInfoReply(reqreply);
     }
 
@@ -1338,10 +1318,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, merge_reply_node_down_after_request_sent) {
         EXPECT_EQ(bucket_id, req->getBuckets()[0]);
 
         auto reqreply = std::make_shared<api::RequestBucketInfoReply>(*req);
-        reqreply->getBucketInfo().push_back(
-                api::RequestBucketInfoReply::Entry(
-                        bucket_id,
-                        api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1))));
+        reqreply->getBucketInfo().emplace_back(bucket_id, api::BucketInfo(10 * (i + 1), 100 * (i +1), 1000 * (i+1)));
         stripe_of_bucket(bucket_id).bucket_db_updater().onRequestBucketInfoReply(reqreply);
     }
 
@@ -1360,7 +1337,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, flush) {
 
     std::vector<api::MergeBucketCommand::Node> nodes;
     for (uint32_t i = 0; i < 3; ++i) {
-        nodes.push_back(api::MergeBucketCommand::Node(i));
+        nodes.emplace_back(i);
     }
 
     api::MergeBucketCommand cmd(makeDocumentBucket(bucket_id), nodes, 0);
@@ -1450,13 +1427,10 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_send_messages) {
               get_sent_nodes("distributor:4 storage:3",
                              "distributor:4 .2.s:d storage:4"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:4 storage:3",
-                             "distributor:4 .0.s:d storage:4"));
-
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:3 storage:3",
-                             "distributor:4 storage:3"));
+    EXPECT_TRUE(get_sent_nodes("distributor:4 storage:3",
+                               "distributor:4 .0.s:d storage:4").empty());
+    EXPECT_TRUE(get_sent_nodes("distributor:3 storage:3",
+                               "distributor:4 storage:3").empty());
 
     EXPECT_EQ(get_node_list({2}),
               get_sent_nodes("distributor:3 storage:3 .2.s:i",
@@ -1470,29 +1444,21 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_send_messages) {
               get_sent_nodes("distributor:3 storage:4 .1.s:d .2.s:i",
                              "distributor:3 storage:5"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:1 storage:3",
-                             "cluster:d"));
-
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:1 storage:3",
-                             "distributor:1 storage:3"));
-
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:1 storage:3",
-                             "cluster:d distributor:1 storage:6"));
-
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:3 storage:3",
-                             "distributor:3 .2.s:m storage:3"));
+    EXPECT_TRUE(get_sent_nodes("distributor:1 storage:3",
+                               "cluster:d").empty());
+    EXPECT_TRUE(get_sent_nodes("distributor:1 storage:3",
+                               "distributor:1 storage:3").empty());
+    EXPECT_TRUE(get_sent_nodes("distributor:1 storage:3",
+                               "cluster:d distributor:1 storage:6").empty());
+    EXPECT_TRUE(get_sent_nodes("distributor:3 storage:3",
+                               "distributor:3 .2.s:m storage:3").empty());
 
     EXPECT_EQ(get_node_list({0, 1, 2}),
               get_sent_nodes("distributor:3 .2.s:m storage:3",
                              "distributor:3 .2.s:d storage:3"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:3 .2.s:m storage:3",
-                           "distributor:3 storage:3"));
+    EXPECT_TRUE(get_sent_nodes("distributor:3 .2.s:m storage:3",
+                               "distributor:3 storage:3").empty());
 
     EXPECT_EQ(get_node_list({0, 1, 2}),
               get_sent_nodes_distribution_changed("distributor:3 storage:3"));
@@ -1501,25 +1467,22 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_send_messages) {
               get_sent_nodes("distributor:10 storage:2",
                              "distributor:10 .1.s:d storage:2"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:2 storage:2",
-                           "distributor:3 .2.s:i storage:2"));
+    EXPECT_TRUE(get_sent_nodes("distributor:2 storage:2",
+                               "distributor:3 .2.s:i storage:2").empty());
 
     EXPECT_EQ(get_node_list({0, 1, 2}),
               get_sent_nodes("distributor:3 storage:3",
-                           "distributor:3 .2.s:s storage:3"));
+                             "distributor:3 .2.s:s storage:3"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:3 .2.s:s storage:3",
-                           "distributor:3 .2.s:d storage:3"));
+    EXPECT_TRUE(get_sent_nodes("distributor:3 .2.s:s storage:3",
+                               "distributor:3 .2.s:d storage:3").empty());
 
     EXPECT_EQ(get_node_list({1}),
               get_sent_nodes("distributor:3 storage:3 .1.s:m",
-                           "distributor:3 storage:3"));
+                             "distributor:3 storage:3"));
 
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:3 storage:3",
-                           "distributor:3 storage:3 .1.s:m"));
+    EXPECT_TRUE(get_sent_nodes("distributor:3 storage:3",
+                               "distributor:3 storage:3 .1.s:m").empty());
 };
 
 TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_receive) {
@@ -1546,10 +1509,7 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_receive) {
 
         auto rep = std::make_shared<RequestBucketInfoReply>(*req);
 
-        rep->getBucketInfo().push_back(
-                RequestBucketInfoReply::Entry(
-                        document::BucketId(16, i),
-                        api::BucketInfo(i, i, i, i, i)));
+        rep->getBucketInfo().emplace_back(document::BucketId(16, i),api::BucketInfo(i, i, i, i, i));
 
         ASSERT_TRUE(state->onRequestBucketInfoReply(rep));
         ASSERT_EQ((i == (sender.commands().size() - 1)), state->done());
@@ -1573,9 +1533,8 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_with_group_down) {
                              "distributor:6 .2.s:d .3.s:d storage:6"));
 
     // But don't fetch if not the entire group is down.
-    EXPECT_EQ("",
-              get_sent_nodes("distributor:6 storage:6",
-                             "distributor:6 .2.s:d storage:6"));
+    EXPECT_EQ("", get_sent_nodes("distributor:6 storage:6",
+                                 "distributor:6 .2.s:d storage:6"));
 }
 
 TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_with_group_down_and_no_handover) {
@@ -1612,23 +1571,16 @@ parse_input_data(const std::string& data,
             if (include_bucket_info) {
                 vespalib::StringTokenizer tok4(tok3[j], "/");
 
-                pending_transition.addNodeInfo(
-                        document::BucketId(16, atoi(tok4[0].data())),
-                        BucketCopy(
-                                timestamp,
-                                node,
-                                api::BucketInfo(
-                                        atoi(tok4[1].data()),
-                                        atoi(tok4[2].data()),
-                                        atoi(tok4[3].data()),
-                                        atoi(tok4[2].data()),
-                                        atoi(tok4[3].data()))));
+                pending_transition.addNodeInfo(document::BucketId(16, atoi(tok4[0].data())),
+                        BucketCopy(timestamp, node,
+                                   api::BucketInfo(atoi(tok4[1].data()),
+                                                   atoi(tok4[2].data()),
+                                                   atoi(tok4[3].data()),
+                                                   atoi(tok4[2].data()),
+                                                   atoi(tok4[3].data()))));
             } else {
-                pending_transition.addNodeInfo(
-                        document::BucketId(16, atoi(tok3[j].data())),
-                        BucketCopy(timestamp,
-                                   node,
-                                   api::BucketInfo(3, 3, 3, 3, 3)));
+                pending_transition.addNodeInfo(document::BucketId(16, atoi(tok3[j].data())),
+                                               BucketCopy(timestamp, node, api::BucketInfo(3, 3, 3, 3, 3)));
             }
         }
     }
diff --git a/storage/src/vespa/storage/bucketdb/bucketcopy.h b/storage/src/vespa/storage/bucketdb/bucketcopy.h
index ca629a6cd8e..5518e9ebe62 100644
--- a/storage/src/vespa/storage/bucketdb/bucketcopy.h
+++ b/storage/src/vespa/storage/bucketdb/bucketcopy.h
@@ -23,8 +23,7 @@ public:
           _info(info),
           _flags(0),
           _node(nodeIdx)
-    {
-    }
+    { }
 
     bool trusted() const noexcept { return _flags & TRUSTED; }
 
diff --git a/storage/src/vespa/storage/bucketdb/bucketinfo.h b/storage/src/vespa/storage/bucketdb/bucketinfo.h
index 9c024c31fd3..fe8fcd340c2 100644
--- a/storage/src/vespa/storage/bucketdb/bucketinfo.h
+++ b/storage/src/vespa/storage/bucketdb/bucketinfo.h
@@ -86,6 +86,7 @@ public:
      * Returns the bucket copy struct for the given node, null if nonexisting
      */
     const BucketCopy* getNode(uint16_t node) const noexcept;
+    uint16_t internal_entry_index(uint16_t node) const noexcept;
 
     /**
      * Returns the number of nodes this entry has.
diff --git a/storage/src/vespa/storage/bucketdb/bucketinfo.hpp b/storage/src/vespa/storage/bucketdb/bucketinfo.hpp
index a8a1069d587..087c8b378b0 100644
--- a/storage/src/vespa/storage/bucketdb/bucketinfo.hpp
+++ b/storage/src/vespa/storage/bucketdb/bucketinfo.hpp
@@ -144,7 +144,18 @@ BucketInfoBase<NodeSeq>::getNode(uint16_t node) const noexcept {
             return &n;
         }
     }
-    return 0;
+    return nullptr;
+}
+
+template <typename NodeSeq>
+uint16_t
+BucketInfoBase<NodeSeq>::internal_entry_index(uint16_t node) const noexcept {
+    for (uint16_t i = 0; i < _nodes.size(); i++) {
+        if (_nodes[i].getNode() == node) {
+            return i;
+        }
+    }
+    return 0xffff; // Not found signal
 }
 
 template <typename NodeSeq>
@@ -221,7 +232,7 @@ BucketInfoBase<NodeSeq>::operator==(const BucketInfoBase<NodeSeq>& other) const
             return false;
         }
 
-        if (!(_nodes[i] == other._nodes[i])) {
+        if (_nodes[i] != other._nodes[i]) {
             return false;
         }
     }
diff --git a/storage/src/vespa/storage/distributor/activecopy.cpp b/storage/src/vespa/storage/distributor/activecopy.cpp
index e9d6d8cca30..42f97c95b95 100644
--- a/storage/src/vespa/storage/distributor/activecopy.cpp
+++ b/storage/src/vespa/storage/distributor/activecopy.cpp
@@ -4,7 +4,6 @@
 #include <vespa/vdslib/distribution/distribution.h>
 #include <vespa/vespalib/stllike/asciistream.h>
 #include <algorithm>
-#include <cassert>
 #include <ostream>
 
 namespace std {
@@ -99,9 +98,8 @@ buildNodeList(const BucketDatabase::Entry& e,vespalib::ConstArrayRef<uint16_t> n
     SmallActiveCopyList result;
     result.reserve(nodeIndexes.size());
     for (uint16_t nodeIndex : nodeIndexes) {
-        const BucketCopy *copy = e->getNode(nodeIndex);
-        assert(copy);
-        result.emplace_back(nodeIndex, *copy, idealState.lookup(nodeIndex));
+        uint16_t entryIndex = e->internal_entry_index(nodeIndex);
+        result.emplace_back(nodeIndex, e->getNodeRef(entryIndex), idealState.lookup(nodeIndex), entryIndex);
     }
     return result;
 }
@@ -154,8 +152,8 @@ ActiveCopy::calculate(const Node2Index & idealState, const lib::Distribution& di
             (inhibited_groups < max_activation_inhibited_out_of_sync_groups) &&
             maybe_majority_info.valid())
         {
-            const auto* candidate = e->getNode(best->_nodeIndex);
-            if (!candidate->getBucketInfo().equalDocumentInfo(maybe_majority_info) && !candidate->active()) {
+            const auto & candidate = e->getNodeRef(best->entryIndex());
+            if (!candidate.getBucketInfo().equalDocumentInfo(maybe_majority_info) && !candidate.active()) {
                 ++inhibited_groups;
                 continue; // Do _not_ add candidate as activation target since it's out of sync with the majority
             }
diff --git a/storage/src/vespa/storage/distributor/activecopy.h b/storage/src/vespa/storage/distributor/activecopy.h
index 91dfb3f0bd0..2085b9632eb 100644
--- a/storage/src/vespa/storage/distributor/activecopy.h
+++ b/storage/src/vespa/storage/distributor/activecopy.h
@@ -19,13 +19,15 @@ public:
         : _nodeIndex(Index::invalid()),
           _ideal(Index::invalid()),
           _doc_count(0),
+          _entryIndex(Index::invalid()),
           _ready(false),
           _active(false)
     { }
-    ActiveCopy(uint16_t node, const BucketCopy & copy, uint16_t ideal) noexcept
+    ActiveCopy(uint16_t node, const BucketCopy & copy, uint16_t ideal, uint16_t entryIndex_in) noexcept
         : _nodeIndex(node),
           _ideal(ideal),
           _doc_count(copy.getDocumentCount()),
+          _entryIndex(entryIndex_in),
           _ready(copy.ready()),
           _active(copy.active())
     { }
@@ -36,12 +38,14 @@ public:
     static ActiveList calculate(const Node2Index & idealState, const lib::Distribution&,
                                 const BucketDatabase::Entry&, uint32_t max_activation_inhibited_out_of_sync_groups);
     uint16_t nodeIndex() const noexcept { return _nodeIndex; }
+    Index entryIndex() const noexcept { return Index(_entryIndex); }
 private:
     friend ActiveStateOrder;
     bool valid_ideal() const noexcept { return _ideal < Index::invalid(); }
     uint16_t _nodeIndex;
     uint16_t _ideal;
     uint32_t _doc_count;
+    uint16_t _entryIndex;  // Index in BucketCopyList
     bool     _ready;
     bool     _active;
 };
diff --git a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
index e7832fd19e5..c7f858de608 100644
--- a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
@@ -70,7 +70,7 @@ PutOperation::insertDatabaseEntryAndScheduleCreateBucket(const OperationTargetLi
                                        _op_ctx.distributor_config().max_activation_inhibited_out_of_sync_groups());
         LOG(debug, "Active copies for bucket %s: %s", entry.getBucketId().toString().c_str(), active.toString().c_str());
         for (uint32_t i=0; i<active.size(); ++i) {
-            BucketCopy copy(*entry->getNode(active[i].nodeIndex()));
+            BucketCopy copy(entry->getNodeRef(active[i].entryIndex()));
             copy.setActive(true);
             entry->updateNode(copy);
         }
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
index 7bec6bbe53a..41767f0e3af 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
@@ -20,8 +20,7 @@ RemoveBucketOperation::onStartInternal(DistributorStripeMessageSender& sender)
 
     BucketDatabase::Entry entry = _bucketSpace->getBucketDatabase().get(getBucketId());
 
-    for (uint32_t i = 0; i < getNodes().size(); ++i) {
-        uint16_t node = getNodes()[i];
+    for (uint16_t node : getNodes()) {
         const BucketCopy* copy(entry->getNode(node));
         if (!copy) {
             LOG(debug, "Node %u was removed between scheduling remove operation and starting it; not sending DeleteBucket to it", node);
@@ -31,7 +30,7 @@ RemoveBucketOperation::onStartInternal(DistributorStripeMessageSender& sender)
         auto msg = std::make_shared<api::DeleteBucketCommand>(getBucket());
         setCommandMeta(*msg);
         msg->setBucketInfo(copy->getBucketInfo());
-        msgs.push_back(std::make_pair(node, msg));
+        msgs.emplace_back(node, msg);
     }
 
     _ok = true;
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/setbucketstateoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/setbucketstateoperation.cpp
index 9547bee6583..531f7f64b68 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/setbucketstateoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/setbucketstateoperation.cpp
@@ -22,10 +22,7 @@ SetBucketStateOperation::~SetBucketStateOperation() = default;
 
 void
 SetBucketStateOperation::enqueueSetBucketStateCommand(uint16_t node, bool active) {
-    auto msg = std::make_shared<api::SetBucketStateCommand>(getBucket(),
-                                                            active
-                                                                ? api::SetBucketStateCommand::ACTIVE
-                                                                : api::SetBucketStateCommand::INACTIVE);
+    auto msg = std::make_shared<api::SetBucketStateCommand>(getBucket(), api::SetBucketStateCommand::toState(active));
     LOG(debug, "Enqueuing %s for %s to node %u", active ? "Activate" : "Deactivate", getBucketId().toString().c_str(), node);
     setCommandMeta(*msg);
     _tracker.queueCommand(std::move(msg), node);
@@ -34,8 +31,8 @@ SetBucketStateOperation::enqueueSetBucketStateCommand(uint16_t node, bool active
 bool
 SetBucketStateOperation::shouldBeActive(uint16_t node) const
 {
-    for (uint32_t i=0, n=_wantedActiveNodes.size(); i<n; ++i) {
-        if (_wantedActiveNodes[i] == node) {
+    for (uint16_t wantedActiveNode : _wantedActiveNodes) {
+        if (wantedActiveNode == node) {
             return true;
         }
     }
@@ -44,8 +41,8 @@ SetBucketStateOperation::shouldBeActive(uint16_t node) const
 
 void
 SetBucketStateOperation::activateNode(DistributorStripeMessageSender& sender) {
-    for (uint32_t i=0; i<_wantedActiveNodes.size(); ++i) {
-        enqueueSetBucketStateCommand(_wantedActiveNodes[i], true);
+    for (uint16_t wantedActiveNode : _wantedActiveNodes) {
+        enqueueSetBucketStateCommand(wantedActiveNode, true);
     }
     _tracker.flushQueue(sender);
     _ok = true;
@@ -54,10 +51,9 @@ SetBucketStateOperation::activateNode(DistributorStripeMessageSender& sender) {
 
 void
 SetBucketStateOperation::deactivateNodes(DistributorStripeMessageSender& sender) {
-    const std::vector<uint16_t>& nodes(getNodes());
-    for (size_t i = 0; i < nodes.size(); ++i) {
-        if (!shouldBeActive(nodes[i])) {
-            enqueueSetBucketStateCommand(nodes[i], false);
+    for (uint16_t node : getNodes()) {
+        if (!shouldBeActive(node)) {
+            enqueueSetBucketStateCommand(node, false);
         }
     }
     _tracker.flushQueue(sender);
@@ -80,12 +76,10 @@ SetBucketStateOperation::onReceive(DistributorStripeMessageSender& sender,
 
     bool deactivate = false;
     if (reply->getResult().success()) {
-        BucketDatabase::Entry entry =
-            _bucketSpace->getBucketDatabase().get(rep.getBucketId());
+        BucketDatabase::Entry entry = _bucketSpace->getBucketDatabase().get(rep.getBucketId());
 
         if (entry.valid()) {
             const BucketCopy* copy = entry->getNode(node);
-
             if (copy) {
                 api::BucketInfo bInfo = copy->getBucketInfo();
 
@@ -96,23 +90,18 @@ SetBucketStateOperation::onReceive(DistributorStripeMessageSender& sender,
                     bInfo.setActive(false);
                 }
 
-                entry->updateNode(
-                        BucketCopy(_manager->operation_context().generate_unique_timestamp(),
-                                   node,
-                                   bInfo).setTrusted(copy->trusted()));
+                entry->updateNode(BucketCopy(_manager->operation_context().generate_unique_timestamp(), node, bInfo)
+                                            .setTrusted(copy->trusted()));
 
                 _bucketSpace->getBucketDatabase().update(entry);
             }
         } else {
             LOG(debug, "%s did not exist when receiving %s",
-                rep.getBucketId().toString().c_str(),
-                rep.toString(true).c_str());
+                rep.getBucketId().toString().c_str(), rep.toString(true).c_str());
         }
     } else {
         LOG(debug, "Failed setting state for %s on node %u: %s",
-            rep.getBucketId().toString().c_str(),
-            node,
-            reply->getResult().toString().c_str());
+            rep.getBucketId().toString().c_str(), node, reply->getResult().toString().c_str());
         _ok = false;
     }
     if (deactivate) {
diff --git a/storage/src/vespa/storage/distributor/statecheckers.cpp b/storage/src/vespa/storage/distributor/statecheckers.cpp
index 478faa38232..c26a5bc1287 100644
--- a/storage/src/vespa/storage/distributor/statecheckers.cpp
+++ b/storage/src/vespa/storage/distributor/statecheckers.cpp
@@ -134,6 +134,22 @@ JoinBucketsStateChecker::isFirstSibling(const document::BucketId& bucketId)
 namespace {
 
 using ConstNodesRef = IdealServiceLayerNodesBundle::ConstNodesRef;
+using Node2Index = IdealServiceLayerNodesBundle::Node2Index;
+
+bool
+equalNodeSet(const Node2Index & node2Index, ConstNodesRef idealState, const BucketDatabase::Entry& dbEntry)
+{
+    if (idealState.size() != dbEntry->getNodeCount()) {
+        return false;
+    }
+    for (uint16_t i = 0; i < dbEntry->getNodeCount(); i++) {
+        const BucketCopy & info = dbEntry->getNodeRef(i);
+        if ( ! node2Index.lookup(info.getNode()).valid() ) {
+            return false;
+        }
+    }
+    return true;
+}
 
 bool
 equalNodeSet(ConstNodesRef idealState, const BucketDatabase::Entry& dbEntry)
@@ -154,7 +170,7 @@ equalNodeSet(ConstNodesRef idealState, const BucketDatabase::Entry& dbEntry)
 bool
 bucketAndSiblingReplicaLocationsEqualIdealState(const StateChecker::Context& context)
 {
-    if (!equalNodeSet(context.idealState(), context.entry)) {
+    if (!equalNodeSet(context.idealStateBundle.nonretired_or_maintenance_to_index(), context.idealState(), context.entry)) {
         return false;
     }
     std::vector<uint16_t> siblingIdealState = context.distribution.getIdealStorageNodes(context.systemState, context.siblingBucket);
@@ -938,27 +954,34 @@ DeleteExtraCopiesStateChecker::check(Context& c) const
     return Result::noMaintenanceNeeded();
 }
 
+namespace {
+
 bool
-BucketStateStateChecker::shouldSkipActivationDueToMaintenance(const ActiveList& activeNodes, const Context& c)
-{
+shouldSkipActivationDueToMaintenanceOrGatherOperationNodes(const ActiveList &activeNodes,
+                                                           const StateChecker::Context &c,
+                                                           std::vector<uint16_t> & operationNodes) {
     for (uint32_t i = 0; i < activeNodes.size(); ++i) {
-        const auto node_index = activeNodes[i].nodeIndex();
-        const BucketCopy* cp(c.entry->getNode(node_index));
-        if (!cp || cp->active()) {
-            continue;
-        }
-        if (!cp->ready()) {
+        const ActiveCopy & active = activeNodes[i];
+        if ( ! active.entryIndex().valid()) continue;
+        const BucketCopy & cp(c.entry->getNodeRef(active.entryIndex()));
+        if (cp.active()) continue;
+
+        const auto node_index = active.nodeIndex();
+        if (!cp.ready()) {
             if (!c.op_ctx.node_supported_features_repo().node_supported_features(node_index).no_implicit_indexing_of_active_buckets) {
                 // If copy is not ready, we don't want to activate it if a node
                 // is set in maintenance. Doing so would imply that we want proton
                 // to start background indexing.
-                return containsMaintenanceNode(c.idealState(), c);
+                if (containsMaintenanceNode(c.idealState(), c)) return true;
             } // else: activation does not imply indexing, so we can safely do it at any time.
         }
+        operationNodes.push_back(node_index);
     }
     return false;
 }
 
+}
+
 /**
  * The copy we want to set active is, in prioritized order:
  *  1. The first ideal state copy that is trusted and ready
@@ -986,19 +1009,18 @@ BucketStateStateChecker::check(Context& c) const
     if (activeNodes.empty()) {
         return Result::noMaintenanceNeeded();
     }
-    if (shouldSkipActivationDueToMaintenance(activeNodes, c)) {
+    std::vector<uint16_t> operationNodes;
+    if (shouldSkipActivationDueToMaintenanceOrGatherOperationNodes(activeNodes, c, operationNodes)) {
         return Result::noMaintenanceNeeded();
     }
-
     vespalib::asciistream reason;
-    std::vector<uint16_t> operationNodes;
-    for (uint32_t i=0; i<activeNodes.size(); ++i) {
-        const BucketCopy* cp = c.entry->getNode(activeNodes[i].nodeIndex());
-        if (cp == nullptr || cp->active()) {
-            continue;
+    for (uint16_t nodeIndex : operationNodes) { // Most of the time empty
+        for (uint32_t i = 0; i < activeNodes.size(); ++i) {
+            const ActiveCopy &active = activeNodes[i];
+            if (nodeIndex == active.nodeIndex()) {
+                reason << "[Setting node " << active.nodeIndex() << " as active: " << active.getReason() << "]";
+            }
         }
-        operationNodes.push_back(activeNodes[i].nodeIndex());
-        reason << "[Setting node " << activeNodes[i].nodeIndex() << " as active: " << activeNodes[i].getReason() << "]";
     }
 
     // Deactivate all copies that are currently marked as active.
diff --git a/storage/src/vespa/storage/distributor/statecheckers.h b/storage/src/vespa/storage/distributor/statecheckers.h
index b6cd5ba0534..88cf8e9712a 100644
--- a/storage/src/vespa/storage/distributor/statecheckers.h
+++ b/storage/src/vespa/storage/distributor/statecheckers.h
@@ -75,7 +75,6 @@ class ActiveList;
 
 class BucketStateStateChecker : public StateChecker
 {
-    static bool shouldSkipActivationDueToMaintenance(const ActiveList& activeList, const Context& c);
 public:
     Result check(Context& c) const override;
     const char* getName() const noexcept override { return "SetBucketState"; }
diff --git a/storage/src/vespa/storageapi/message/bucket.h b/storage/src/vespa/storageapi/message/bucket.h
index e02b8fcd672..80565334331 100644
--- a/storage/src/vespa/storageapi/message/bucket.h
+++ b/storage/src/vespa/storageapi/message/bucket.h
@@ -106,7 +106,8 @@ public:
         uint16_t index;
         bool sourceOnly;
 
-        Node(uint16_t index_, bool sourceOnly_ = false) noexcept
+        Node(uint16_t index_) noexcept : Node(index_, false) { }
+        Node(uint16_t index_, bool sourceOnly_) noexcept
             : index(index_), sourceOnly(sourceOnly_) {}
 
         bool operator==(const Node& n) const noexcept
@@ -471,21 +472,15 @@ public:
 class SetBucketStateCommand : public MaintenanceCommand
 {
 public:
-    enum BUCKET_STATE
-    {
-        INACTIVE,
-        ACTIVE
-    };
-private:
-    BUCKET_STATE _state;
-public:
+    enum BUCKET_STATE { INACTIVE, ACTIVE };
     SetBucketStateCommand(const document::Bucket &bucket, BUCKET_STATE state);
     void print(std::ostream& out, bool verbose, const std::string& indent) const override;
     BUCKET_STATE getState() const { return _state; }
-    DECLARE_STORAGECOMMAND(SetBucketStateCommand, onSetBucketState)
+    static BUCKET_STATE toState(bool active) noexcept { return active ? ACTIVE : INACTIVE; }
+    DECLARE_STORAGECOMMAND(SetBucketStateCommand, onSetBucketState);
 private:
-
     vespalib::string getSummary() const override;
+    BUCKET_STATE _state;
 };
 
 /**
diff --git a/storage/src/vespa/storageapi/messageapi/bucketcommand.cpp b/storage/src/vespa/storageapi/messageapi/bucketcommand.cpp
index c75267f560d..f202ce9da7d 100644
--- a/storage/src/vespa/storageapi/messageapi/bucketcommand.cpp
+++ b/storage/src/vespa/storageapi/messageapi/bucketcommand.cpp
@@ -7,10 +7,9 @@ using document::Bucket;
 using document::BucketId;
 using document::BucketSpace;
 
-namespace storage {
-namespace api {
+namespace storage::api {
 
-BucketCommand::BucketCommand(const MessageType& type, const Bucket &bucket)
+BucketCommand::BucketCommand(const MessageType& type, const Bucket &bucket) noexcept
     : StorageCommand(type),
       _bucket(bucket),
       _originalBucket()
@@ -42,5 +41,4 @@ BucketCommand::print(std::ostream& out,
     }
 }
 
-} // api
-} // storage
+}
diff --git a/storage/src/vespa/storageapi/messageapi/bucketcommand.h b/storage/src/vespa/storageapi/messageapi/bucketcommand.h
index 605653681b5..f1d5cea377c 100644
--- a/storage/src/vespa/storageapi/messageapi/bucketcommand.h
+++ b/storage/src/vespa/storageapi/messageapi/bucketcommand.h
@@ -18,7 +18,7 @@ class BucketCommand : public StorageCommand {
     document::BucketId _originalBucket;
 
 protected:
-    BucketCommand(const MessageType& type, const document::Bucket &bucket);
+    BucketCommand(const MessageType& type, const document::Bucket &bucket) noexcept;
 
 public:
     DECLARE_POINTER_TYPEDEFS(BucketCommand);
diff --git a/storage/src/vespa/storageapi/messageapi/bucketinfocommand.h b/storage/src/vespa/storageapi/messageapi/bucketinfocommand.h
index 0f6627328a9..0675e639096 100644
--- a/storage/src/vespa/storageapi/messageapi/bucketinfocommand.h
+++ b/storage/src/vespa/storageapi/messageapi/bucketinfocommand.h
@@ -18,7 +18,7 @@ namespace storage::api {
 
 class BucketInfoCommand : public BucketCommand {
 protected:
-    BucketInfoCommand(const MessageType& type, const document::Bucket &bucket)
+    BucketInfoCommand(const MessageType& type, const document::Bucket &bucket) noexcept
         : BucketCommand(type, bucket) {}
 
 public:
diff --git a/storage/src/vespa/storageapi/messageapi/maintenancecommand.h b/storage/src/vespa/storageapi/messageapi/maintenancecommand.h
index 6bb36d0d32f..4b4612123a5 100644
--- a/storage/src/vespa/storageapi/messageapi/maintenancecommand.h
+++ b/storage/src/vespa/storageapi/messageapi/maintenancecommand.h
@@ -3,8 +3,7 @@
 
 #include "bucketinfocommand.h"
 
-namespace storage {
-namespace api {
+namespace storage::api {
 
 class MaintenanceCommand : public BucketInfoCommand
 {
@@ -13,10 +12,10 @@ public:
         : BucketInfoCommand(type, bucket)
     {}
     MaintenanceCommand(const MaintenanceCommand &) = default;
-    MaintenanceCommand(MaintenanceCommand &&) = default;
+    MaintenanceCommand(MaintenanceCommand &&) noexcept = default;
     MaintenanceCommand & operator = (const MaintenanceCommand &) = delete;
-    MaintenanceCommand & operator = (MaintenanceCommand &&) = delete;
-    ~MaintenanceCommand();
+    MaintenanceCommand & operator = (MaintenanceCommand &&) noexcept = delete;
+    ~MaintenanceCommand() override;
 
     const vespalib::string& getReason() const { return _reason; };
     void setReason(vespalib::stringref reason) { _reason = reason; };
@@ -25,4 +24,3 @@ protected:
 };
 
 }
-}
diff --git a/storage/src/vespa/storageapi/messageapi/storagecommand.cpp b/storage/src/vespa/storageapi/messageapi/storagecommand.cpp
index 249e08362d4..c0da654627c 100644
--- a/storage/src/vespa/storageapi/messageapi/storagecommand.cpp
+++ b/storage/src/vespa/storageapi/messageapi/storagecommand.cpp
@@ -17,7 +17,7 @@ StorageCommand::StorageCommand(const StorageCommand& other)
 {
 }
 
-StorageCommand::StorageCommand(const MessageType& type, Priority p)
+StorageCommand::StorageCommand(const MessageType& type, Priority p) noexcept
     : StorageMessage(type, generateMsgId(), 0),
       _timeout(MAX_TIMEOUT),
       _sourceIndex(0xFFFF)
diff --git a/storage/src/vespa/storageapi/messageapi/storagecommand.h b/storage/src/vespa/storageapi/messageapi/storagecommand.h
index 06817a65b05..e5810ec638a 100644
--- a/storage/src/vespa/storageapi/messageapi/storagecommand.h
+++ b/storage/src/vespa/storageapi/messageapi/storagecommand.h
@@ -20,7 +20,8 @@ class StorageCommand : public StorageMessage {
 
 protected:
     StorageCommand(const StorageCommand& other);
-    explicit StorageCommand(const MessageType& type, Priority p = NORMAL);
+    explicit StorageCommand(const MessageType& type) noexcept : StorageCommand(type, NORMAL) { }
+    explicit StorageCommand(const MessageType& type, Priority p) noexcept;
 
 public:
     DECLARE_POINTER_TYPEDEFS(StorageCommand);