From 190a32ada5a9d536d72462ef91b8d5322cc950e5 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Thu, 10 Aug 2023 20:17:55 +0000 Subject: Minor code health. --- .../distributor/distributor_stripe_component.cpp | 88 +++++----------------- .../distributor/distributor_stripe_component.h | 31 ++------ .../distributor/operationtargetresolverimpl.cpp | 58 +++++--------- .../vdslib/distribution/idealnodecalculator.h | 35 ++++----- .../distribution/idealnodecalculatorimpl.cpp | 9 ++- 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& unavailableNodes, - const lib::ClusterState& s, - const document::Bucket& bucket, - const std::vector& 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& changed_nodes, - uint32_t update_flags) +DistributorStripeComponent::update_bucket_database(const document::Bucket& bucket, + const std::vector& 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& nodes) +DistributorStripeComponent::remove_nodes_from_bucket_database(const document::Bucket& bucket, const std::vector& 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(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(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(changed_node),update_flags); } /** * Adds the given copies to the bucket database. */ - void update_bucket_database(const document::Bucket& bucket, - const std::vector& changed_nodes, - uint32_t update_flags) override; + void update_bucket_database(const document::Bucket& bucket, const std::vector& 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& nodes) override; + void remove_nodes_from_bucket_database(const document::Bucket& bucket, const std::vector& 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 parse_selection(const vespalib::string& selection) const override; private: - void enumerateUnavailableNodes( - std::vector& unavailableNodes, - const lib::ClusterState& s, - const document::Bucket& bucket, - const std::vector& 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& 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 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; - 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 _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); + } } } -- cgit v1.2.3