diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2023-08-10 20:17:55 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2023-08-10 20:17:55 +0000 |
commit | 190a32ada5a9d536d72462ef91b8d5322cc950e5 (patch) | |
tree | 85dd9e61d66f6b01896887940946bca9bcd7b64f | |
parent | dc46fc272ddb75f97357ad5602e6217821310ea7 (diff) |
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); + } } } |