Merge pull request #28023 from vespa-engine/balder/minor-layout-cleanup

Minor code health.

5 files changed, 65 insertions, 156 deletions

diff --git a/storage/src/vespa/storage/distributor/distributor_stripe_component.cpp b/storage/src/vespa/storage/distributor/distributor_stripe_component.cpp
index 1121c5071c0..16cc887096f 100644
--- a/storage/src/vespa/storage/distributor/distributor_stripe_component.cpp
+++ b/storage/src/vespa/storage/distributor/distributor_stripe_component.cpp
@@ -17,12 +17,11 @@ using document::BucketSpace;
 
 namespace storage::distributor {
 
-DistributorStripeComponent::DistributorStripeComponent(
-        DistributorStripeInterface& distributor,
-        DistributorBucketSpaceRepo& bucketSpaceRepo,
-        DistributorBucketSpaceRepo& readOnlyBucketSpaceRepo,
-        DistributorComponentRegister& compReg,
-        const std::string& name)
+DistributorStripeComponent::DistributorStripeComponent(DistributorStripeInterface& distributor,
+                                                       DistributorBucketSpaceRepo& bucketSpaceRepo,
+                                                       DistributorBucketSpaceRepo& readOnlyBucketSpaceRepo,
+                                                       DistributorComponentRegister& compReg,
+                                                       const std::string& name)
     : storage::DistributorComponent(compReg, name),
       _distributor(distributor),
       _bucketSpaceRepo(bucketSpaceRepo),
@@ -44,30 +43,6 @@ DistributorStripeComponent::sendUp(const api::StorageMessage::SP& msg)
     _distributor.getMessageSender().sendUp(msg);
 }
 
-void
-DistributorStripeComponent::enumerateUnavailableNodes(
-        std::vector<uint16_t>& unavailableNodes,
-        const lib::ClusterState& s,
-        const document::Bucket& bucket,
-        const std::vector<BucketCopy>& candidates) const
-{
-    const auto* up_states = storage_node_up_states();
-    for (uint32_t i = 0; i < candidates.size(); ++i) {
-        const BucketCopy& copy(candidates[i]);
-        const lib::NodeState& ns(
-                s.getNodeState(lib::Node(lib::NodeType::STORAGE, copy.getNode())));
-        if (!ns.getState().oneOf(up_states)) {
-            LOG(debug,
-                "Trying to add a bucket copy to %s whose node is marked as "
-                "down in the cluster state: %s. Ignoring it since no zombies "
-                "are allowed!",
-                bucket.toString().c_str(),
-                copy.toString().c_str());
-            unavailableNodes.emplace_back(copy.getNode());
-        }
-    }
-}
-
 namespace {
 
 /**
@@ -97,8 +72,7 @@ UpdateBucketDatabaseProcessor::UpdateBucketDatabaseProcessor(const framework::Cl
 UpdateBucketDatabaseProcessor::~UpdateBucketDatabaseProcessor() = default;
 
 BucketDatabase::Entry
-UpdateBucketDatabaseProcessor::create_entry(const document::BucketId &bucket) const
-{
+UpdateBucketDatabaseProcessor::create_entry(const document::BucketId &bucket) const {
     return BucketDatabase::Entry(bucket, BucketInfo());
 }
 
@@ -125,21 +99,16 @@ UpdateBucketDatabaseProcessor::process_entry(BucketDatabase::Entry &entry) const
 }
 
 void
-DistributorStripeComponent::update_bucket_database(
-        const document::Bucket& bucket,
-        const std::vector<BucketCopy>& changed_nodes,
-        uint32_t update_flags)
+DistributorStripeComponent::update_bucket_database(const document::Bucket& bucket,
+                                                   const std::vector<BucketCopy>& changed_nodes, uint32_t update_flags)
 {
     auto &bucketSpace(_bucketSpaceRepo.get(bucket.getBucketSpace()));
     assert(!(bucket.getBucketId() == document::BucketId()));
 
     BucketOwnership ownership(bucketSpace.check_ownership_in_pending_and_current_state(bucket.getBucketId()));
     if (!ownership.isOwned()) {
-        LOG(debug,
-            "Trying to add %s to database that we do not own according to "
-            "cluster state '%s' - ignoring!",
-            bucket.toString().c_str(),
-            ownership.getNonOwnedState().toString().c_str());
+        LOG(debug, "Trying to add %s to database that we do not own according to cluster state '%s' - ignoring!",
+            bucket.toString().c_str(), ownership.getNonOwnedState().toString().c_str());
         return;
     }
 
@@ -184,8 +153,7 @@ DistributorStripeComponent::node_address(uint16_t node_index) const noexcept
 
 // Implements DistributorStripeOperationContext
 void
-DistributorStripeComponent::remove_nodes_from_bucket_database(const document::Bucket& bucket,
-                                                              const std::vector<uint16_t>& nodes)
+DistributorStripeComponent::remove_nodes_from_bucket_database(const document::Bucket& bucket, const std::vector<uint16_t>& nodes)
 {
     auto &bucketSpace(_bucketSpaceRepo.get(bucket.getBucketSpace()));
     BucketDatabase::Entry dbentry = bucketSpace.getBucketDatabase().get(bucket.getBucketId());
@@ -193,21 +161,15 @@ DistributorStripeComponent::remove_nodes_from_bucket_database(const document::Bu
     if (dbentry.valid()) {
         for (uint32_t i = 0; i < nodes.size(); ++i) {
             if (dbentry->removeNode(nodes[i])) {
-                LOG(debug,
-                    "Removed node %d from bucket %s. %u copies remaining",
-                    nodes[i],
-                    bucket.toString().c_str(),
-                    dbentry->getNodeCount());
+                LOG(debug, "Removed node %d from bucket %s. %u copies remaining",
+                    nodes[i], bucket.toString().c_str(), dbentry->getNodeCount());
             }
         }
 
         if (dbentry->getNodeCount() != 0) {
             bucketSpace.getBucketDatabase().update(dbentry);
         } else {
-            LOG(debug,
-                "After update, bucket %s now has no copies. "
-                "Removing from database.",
-                bucket.toString().c_str());
+            LOG(debug, "After update, bucket %s now has no copies. Removing from database.", bucket.toString().c_str());
 
             bucketSpace.getBucketDatabase().remove(bucket.getBucketId());
         }
@@ -218,7 +180,6 @@ document::BucketId
 DistributorStripeComponent::make_split_bit_constrained_bucket_id(const document::DocumentId& doc_id) const
 {
     document::BucketId id(getBucketIdFactory().getBucketId(doc_id));
-
     id.setUsedBits(_distributor.getConfig().getMinimalBucketSplit());
     return id.stripUnused();
 }
@@ -239,28 +200,18 @@ DistributorStripeComponent::get_sibling(const document::BucketId& bid) const
         zeroBucket = document::BucketId(1, 0);
         oneBucket = document::BucketId(1, 1);
     } else {
-        document::BucketId joinedBucket = document::BucketId(
-                bid.getUsedBits() - 1,
-                bid.getId());
-
-        zeroBucket = document::BucketId(
-                bid.getUsedBits(),
-                joinedBucket.getId());
-
+        document::BucketId joinedBucket = document::BucketId(bid.getUsedBits() - 1,bid.getId());
+        zeroBucket = document::BucketId(bid.getUsedBits(), joinedBucket.getId());
         uint64_t hiBit = 1;
         hiBit <<= (bid.getUsedBits() - 1);
-        oneBucket = document::BucketId(
-                bid.getUsedBits(),
-                joinedBucket.getId() | hiBit);
+        oneBucket = document::BucketId(bid.getUsedBits(), joinedBucket.getId() | hiBit);
     }
 
     return (zeroBucket == bid) ? oneBucket : zeroBucket;
 }
 
 bool
-DistributorStripeComponent::has_pending_message(uint16_t node_index,
-                                                const document::Bucket& bucket,
-                                                uint32_t message_type) const
+DistributorStripeComponent::has_pending_message(uint16_t node_index, const document::Bucket& bucket, uint32_t message_type) const
 {
     const auto& sender = static_cast<const DistributorStripeMessageSender&>(getDistributor());
     return sender.getPendingMessageTracker().hasPendingMessage(node_index, bucket, message_type);
@@ -275,8 +226,7 @@ DistributorStripeComponent::cluster_state_bundle() const
 bool
 DistributorStripeComponent::storage_node_is_up(document::BucketSpace bucket_space, uint32_t node_index) const
 {
-    const lib::NodeState& ns = cluster_state_bundle().getDerivedClusterState(bucket_space)->getNodeState(
-            lib::Node(lib::NodeType::STORAGE, node_index));
+    const auto & ns = cluster_state_bundle().getDerivedClusterState(bucket_space)->getNodeState(lib::Node(lib::NodeType::STORAGE, node_index));
 
     return ns.getState().oneOf(storage_node_up_states());
 }
diff --git a/storage/src/vespa/storage/distributor/distributor_stripe_component.h b/storage/src/vespa/storage/distributor/distributor_stripe_component.h
index 5bcf9eec76d..8bf507f3fac 100644
--- a/storage/src/vespa/storage/distributor/distributor_stripe_component.h
+++ b/storage/src/vespa/storage/distributor/distributor_stripe_component.h
@@ -68,20 +68,14 @@ public:
     /**
      * Simple API for the common case of modifying a single node.
      */
-    void update_bucket_database(const document::Bucket& bucket,
-                                const BucketCopy& changed_node,
-                                uint32_t update_flags) override {
-        update_bucket_database(bucket,
-                               toVector<BucketCopy>(changed_node),
-                               update_flags);
+    void update_bucket_database(const document::Bucket& bucket, const BucketCopy& changed_node, uint32_t update_flags) override {
+        update_bucket_database(bucket, toVector<BucketCopy>(changed_node),update_flags);
     }
 
     /**
      * Adds the given copies to the bucket database.
      */
-    void update_bucket_database(const document::Bucket& bucket,
-                                const std::vector<BucketCopy>& changed_nodes,
-                                uint32_t update_flags) override;
+    void update_bucket_database(const document::Bucket& bucket, const std::vector<BucketCopy>& changed_nodes, uint32_t update_flags) override;
 
     /**
      * Removes a copy from the given bucket from the bucket database.
@@ -97,8 +91,7 @@ public:
      * If the resulting bucket is empty afterwards, removes the entire
      * bucket entry from the bucket database.
      */
-    void remove_nodes_from_bucket_database(const document::Bucket& bucket,
-                                           const std::vector<uint16_t>& nodes) override;
+    void remove_nodes_from_bucket_database(const document::Bucket& bucket, const std::vector<uint16_t>& nodes) override;
 
     const DistributorBucketSpaceRepo& bucket_space_repo() const noexcept override {
         return _bucketSpaceRepo;
@@ -129,9 +122,7 @@ public:
     const DistributorConfiguration& distributor_config() const noexcept override {
         return getDistributor().getConfig();
     }
-    void send_inline_split_if_bucket_too_large(document::BucketSpace bucket_space,
-                                               const BucketDatabase::Entry& entry,
-                                               uint8_t pri) override {
+    void send_inline_split_if_bucket_too_large(document::BucketSpace bucket_space, const BucketDatabase::Entry& entry, uint8_t pri) override {
         getDistributor().checkBucketForSplit(bucket_space, entry, pri);
     }
     OperationRoutingSnapshot read_snapshot_for_bucket(const document::Bucket& bucket) const override {
@@ -143,9 +134,7 @@ public:
     const PendingMessageTracker& pending_message_tracker() const noexcept override {
         return getDistributor().getPendingMessageTracker();
     }
-    bool has_pending_message(uint16_t node_index,
-                             const document::Bucket& bucket,
-                             uint32_t message_type) const override;
+    bool has_pending_message(uint16_t node_index, const document::Bucket& bucket, uint32_t message_type) const override;
     const lib::ClusterState* pending_cluster_state_or_null(const document::BucketSpace& bucket_space) const override {
         return getDistributor().pendingClusterStateOrNull(bucket_space);
     }
@@ -171,15 +160,7 @@ public:
     std::unique_ptr<document::select::Node> parse_selection(const vespalib::string& selection) const override;
 
 private:
-    void enumerateUnavailableNodes(
-            std::vector<uint16_t>& unavailableNodes,
-            const lib::ClusterState& s,
-            const document::Bucket& bucket,
-            const std::vector<BucketCopy>& candidates) const;
     DistributorStripeInterface& _distributor;
-
-protected:
-
     DistributorBucketSpaceRepo& _bucketSpaceRepo;
     DistributorBucketSpaceRepo& _readOnlyBucketSpaceRepo;
 };
diff --git a/storage/src/vespa/storage/distributor/operationtargetresolverimpl.cpp b/storage/src/vespa/storage/distributor/operationtargetresolverimpl.cpp
index fcd27fdab74..736d8c692e3 100644
--- a/storage/src/vespa/storage/distributor/operationtargetresolverimpl.cpp
+++ b/storage/src/vespa/storage/distributor/operationtargetresolverimpl.cpp
@@ -23,9 +23,8 @@ make_node_list(const std::vector<uint16_t>& nodes)
 
 }
 
-BucketInstance::BucketInstance(
-        const document::BucketId& id, const api::BucketInfo& info,
-        lib::Node node, uint16_t idealLocationPriority, bool trusted, bool exist)
+BucketInstance::BucketInstance(const document::BucketId& id, const api::BucketInfo& info, lib::Node node,
+                               uint16_t idealLocationPriority, bool trusted, bool exist)
     : _bucket(id), _info(info), _node(node),
       _idealLocationPriority(idealLocationPriority), _trusted(trusted), _exist(exist)
 {
@@ -39,12 +38,8 @@ BucketInstance::print(vespalib::asciistream& out, const PrintProperties&) const
 
     std::ostringstream ost;
     ost << std::hex << _bucket.getId();
-    out << "(" << ost.str() << ", "
-        << infoString << ", node " << _node.getIndex()
-        << ", ideal " << _idealLocationPriority
-        << (_trusted ? ", trusted" : "")
-        << (_exist ? "" : ", new copy")
-        << ")";
+    out << "(" << ost.str() << ", " << infoString << ", node " << _node.getIndex() << ", ideal " << _idealLocationPriority
+        << (_trusted ? ", trusted" : "") << (_exist ? "" : ", new copy") << ")";
 }
 
 bool
@@ -71,8 +66,7 @@ BucketInstanceList::populate(const document::BucketId& specificId, const Distrib
     std::vector<BucketDatabase::Entry> entries;
     db.getParents(specificId, entries);
     for (const auto & entry : entries) {
-        lib::IdealNodeList idealNodes(make_node_list(distributor_bucket_space.get_ideal_service_layer_nodes_bundle(
-                entry.getBucketId()).available_nonretired_or_maintenance_nodes()));
+        lib::IdealNodeList idealNodes(make_node_list(distributor_bucket_space.get_ideal_service_layer_nodes_bundle(entry.getBucketId()).available_nonretired_or_maintenance_nodes()));
         add(entry, idealNodes);
     }
 }
@@ -100,41 +94,34 @@ BucketInstanceList::limitToRedundancyCopies(uint16_t redundancy)
 }
 
 document::BucketId
-BucketInstanceList::leastSpecificLeafBucketInSubtree(
-        const document::BucketId& candidateId,
-        const document::BucketId& mostSpecificId,
-        const BucketDatabase& db) const
+BucketInstanceList::leastSpecificLeafBucketInSubtree(const document::BucketId& candidateId,
+                                                     const document::BucketId& mostSpecificId,
+                                                     const BucketDatabase& db) const
 {
     assert(candidateId.contains(mostSpecificId));
     document::BucketId treeNode = candidateId;
     // treeNode may reach at most 58 bits since buckets at 58 bits by definition
     // cannot have any children.
     while (db.childCount(treeNode) != 0) {
-        treeNode = document::BucketId(treeNode.getUsedBits() + 1,
-                                      mostSpecificId.getRawId());
+        treeNode = document::BucketId(treeNode.getUsedBits() + 1, mostSpecificId.getRawId());
     }
     assert(treeNode.contains(mostSpecificId));
     return treeNode;
 }
 
 void
-BucketInstanceList::extendToEnoughCopies(
-        const DistributorBucketSpace& distributor_bucket_space,
-        const BucketDatabase& db,
-        const document::BucketId& targetIfNonPreExisting,
-        const document::BucketId& mostSpecificId)
+BucketInstanceList::extendToEnoughCopies(const DistributorBucketSpace& distributor_bucket_space,
+                                         const BucketDatabase& db,
+                                         const document::BucketId& targetIfNonPreExisting,
+                                         const document::BucketId& mostSpecificId)
 {
-    document::BucketId newTarget(_instances.empty() ? targetIfNonPreExisting
-                                                    : _instances[0]._bucket);
+    document::BucketId newTarget(_instances.empty() ? targetIfNonPreExisting : _instances[0]._bucket);
     newTarget = leastSpecificLeafBucketInSubtree(newTarget, mostSpecificId, db);
 
-    lib::IdealNodeList idealNodes(make_node_list(
-            distributor_bucket_space.get_ideal_service_layer_nodes_bundle(newTarget).available_nonretired_nodes()));
+    lib::IdealNodeList idealNodes(make_node_list(distributor_bucket_space.get_ideal_service_layer_nodes_bundle(newTarget).available_nonretired_nodes()));
     for (uint32_t i=0; i<idealNodes.size(); ++i) {
         if (!contains(idealNodes[i])) {
-            _instances.push_back(BucketInstance(
-                    newTarget, api::BucketInfo(), idealNodes[i],
-                    i, false, false));
+            _instances.emplace_back(newTarget, api::BucketInfo(), idealNodes[i], i, false, false);
         }
     }
 }
@@ -187,22 +174,17 @@ struct InstanceOrder {
 } // anonymous
 
 BucketInstanceList
-OperationTargetResolverImpl::getAllInstances(OperationType type,
-                                             const document::BucketId& id)
+OperationTargetResolverImpl::getAllInstances(OperationType type, const document::BucketId& id)
 {
     BucketInstanceList instances;
     if (type == PUT) {
         instances.populate(id, _distributor_bucket_space, _bucketDatabase);
         instances.sort(InstanceOrder());
         instances.removeNodeDuplicates();
-        instances.extendToEnoughCopies(
-                _distributor_bucket_space,
-                _bucketDatabase,
-                _bucketDatabase.getAppropriateBucket(_minUsedBucketBits, id),
-                id);
+        instances.extendToEnoughCopies(_distributor_bucket_space, _bucketDatabase,
+                                       _bucketDatabase.getAppropriateBucket(_minUsedBucketBits, id), id);
     } else {
-        throw vespalib::IllegalArgumentException(
-                "Unsupported operation type given", VESPA_STRLOC);
+        throw vespalib::IllegalArgumentException("Unsupported operation type given", VESPA_STRLOC);
     }
     return instances;
 }
diff --git a/vdslib/src/vespa/vdslib/distribution/idealnodecalculator.h b/vdslib/src/vespa/vdslib/distribution/idealnodecalculator.h
index bc42df1b49c..4eb8f7e04ae 100644
--- a/vdslib/src/vespa/vdslib/distribution/idealnodecalculator.h
+++ b/vdslib/src/vespa/vdslib/distribution/idealnodecalculator.h
@@ -22,25 +22,20 @@ class ClusterState;
  * unneeded details, and make it easily printable.
  */
 class IdealNodeList : public document::Printable {
-    std::vector<Node> _idealNodes;
-
 public:
-    IdealNodeList();
+    IdealNodeList() noexcept;
     ~IdealNodeList();
 
     void push_back(const Node& node) {
         _idealNodes.push_back(node);
     }
 
-    const Node& operator[](uint32_t i) const { return _idealNodes[i]; }
-    uint32_t size() const { return _idealNodes.size(); }
-    bool contains(const Node& n) const {
-        for (uint32_t i=0; i<_idealNodes.size(); ++i) {
-            if (n == _idealNodes[i]) return true;
-        }
-        return false;
+    const Node& operator[](uint32_t i) const noexcept { return _idealNodes[i]; }
+    uint32_t size() const noexcept { return _idealNodes.size(); }
+    bool contains(const Node& n) const noexcept {
+        return indexOf(n) != 0xffff;
     }
-    uint16_t indexOf(const Node& n) const {
+    uint16_t indexOf(const Node& n) const noexcept {
         for (uint16_t i=0; i<_idealNodes.size(); ++i) {
             if (n == _idealNodes[i]) return i;
         }
@@ -48,6 +43,8 @@ public:
     }
 
     void print(std::ostream& out, bool, const std::string &) const override;
+private:
+    std::vector<Node> _idealNodes;
 };
 
 /**
@@ -64,17 +61,15 @@ public:
 
     virtual ~IdealNodeCalculator() = default;
 
-    virtual IdealNodeList getIdealNodes(const NodeType&,
-                                        const document::BucketId&,
-                                        UpStates upStates = UpInit) const = 0;
+    virtual IdealNodeList getIdealNodes(const NodeType&, const document::BucketId&, UpStates upStates = UpInit) const = 0;
 
     // Wrapper functions to make prettier call if nodetype is given.
-    IdealNodeList getIdealDistributorNodes(const document::BucketId& bucket,
-                                           UpStates upStates = UpInit) const
-        { return getIdealNodes(NodeType::DISTRIBUTOR, bucket, upStates); }
-    IdealNodeList getIdealStorageNodes(const document::BucketId& bucket,
-                                       UpStates upStates = UpInit) const
-        { return getIdealNodes(NodeType::STORAGE, bucket, upStates); }
+    IdealNodeList getIdealDistributorNodes(const document::BucketId& bucket, UpStates upStates = UpInit) const {
+        return getIdealNodes(NodeType::DISTRIBUTOR, bucket, upStates);
+    }
+    IdealNodeList getIdealStorageNodes(const document::BucketId& bucket, UpStates upStates = UpInit) const {
+        return getIdealNodes(NodeType::STORAGE, bucket, upStates);
+    }
 };
 
 
diff --git a/vdslib/src/vespa/vdslib/distribution/idealnodecalculatorimpl.cpp b/vdslib/src/vespa/vdslib/distribution/idealnodecalculatorimpl.cpp
index da34ec4526a..86123f47d6f 100644
--- a/vdslib/src/vespa/vdslib/distribution/idealnodecalculatorimpl.cpp
+++ b/vdslib/src/vespa/vdslib/distribution/idealnodecalculatorimpl.cpp
@@ -8,7 +8,7 @@
 
 namespace storage::lib {
 
-IdealNodeList::IdealNodeList() = default;
+IdealNodeList::IdealNodeList() noexcept = default;
 IdealNodeList::~IdealNodeList() = default;
 
 void
@@ -63,9 +63,10 @@ IdealNodeCalculatorImpl::initUpStateMapping() {
     _upStates[UpInit] = "ui";
     _upStates[UpInitMaintenance] = "uim";
     for (uint32_t i=0; i<_upStates.size(); ++i) {
-        if (_upStates[i] == 0) throw vespalib::IllegalStateException(
-                "Failed to initialize up state. Code likely not updated "
-                "after another upstate was added.", VESPA_STRLOC);
+        if (_upStates[i] == 0) {
+            throw vespalib::IllegalStateException("Failed to initialize up state. Code likely not updated "
+                                                  "after another upstate was added.", VESPA_STRLOC);
+        }
     }
 }