diff options
Diffstat (limited to 'storage')
5 files changed, 44 insertions, 39 deletions
diff --git a/storage/src/tests/distributor/mergeoperationtest.cpp b/storage/src/tests/distributor/mergeoperationtest.cpp index 7a90299bfb3..1026ea2855e 100644 --- a/storage/src/tests/distributor/mergeoperationtest.cpp +++ b/storage/src/tests/distributor/mergeoperationtest.cpp @@ -315,7 +315,7 @@ TEST_F(MergeOperationTest, allow_deleting_active_source_only_replica) { _sender.getLastCommand(true)); } -TEST_F(MergeOperationTest, MarkRedundantTrustedCopiesAsSourceOnly) { +TEST_F(MergeOperationTest, mark_redundant_trusted_copies_as_source_only) { // This test uses the same distribution as testGenerateNodeList(), i.e. // an ideal state sequence of [3, 5, 7, 6, 8, 0, 9, 2, 1, 4] @@ -415,6 +415,21 @@ TEST_F(MergeOperationTest, merge_operation_is_blocked_by_any_busy_target_node) { EXPECT_TRUE(op.isBlocked(*_pendingTracker, _operation_sequencer)); } + +TEST_F(MergeOperationTest, global_bucket_merges_are_not_blocked_by_busy_nodes) { + getClock().setAbsoluteTimeInSeconds(10); + document::BucketId bucket_id(16, 1); + addNodesToBucketDB(bucket_id, "0=10/1/1/t,1=20/1/1,2=10/1/1/t"); + enableDistributorClusterState("distributor:1 storage:3"); + document::Bucket global_bucket(document::FixedBucketSpaces::global_space(), bucket_id); + MergeOperation op(BucketAndNodes(global_bucket, toVector<uint16_t>(0, 1, 2))); + op.setIdealStateManager(&getIdealStateManager()); + + // Node 1 is included in operation node set but should not cause a block of global bucket merge + _pendingTracker->getNodeInfo().setBusy(0, std::chrono::seconds(10)); + EXPECT_FALSE(op.isBlocked(*_pendingTracker, _operation_sequencer)); +} + TEST_F(MergeOperationTest, merge_operation_is_blocked_by_locked_bucket) { getClock().setAbsoluteTimeInSeconds(10); addNodesToBucketDB(document::BucketId(16, 1), "0=10/1/1/t,1=20/1/1,2=10/1/1/t"); diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp index 142ff72bc79..1a48df0fd7c 100644 --- a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp @@ -9,8 +9,8 @@ #include <vespa/log/log.h> LOG_SETUP(".distributor.operation"); -using namespace storage; -using namespace storage::distributor; +namespace storage::distributor { + using document::BucketSpace; const uint32_t IdealStateOperation::MAINTENANCE_MESSAGE_TYPES[] = @@ -85,7 +85,7 @@ IdealStateOperation::setIdealStateManager(IdealStateManager* manager) { void IdealStateOperation::done() { - if (_manager != NULL) { + if (_manager) { if (ok()) { _manager->getMetrics().operations[getType()]->ok.inc(1); } else { @@ -107,35 +107,6 @@ IdealStateOperation::setCommandMeta(api::MaintenanceCommand& cmd) const cmd.setReason(_detailedReason); } -std::string -IdealStateOperation::toXML(framework::Clock& clock) const -{ - std::ostringstream ost; - - ost << "<operation bucketid=\"" << getBucketId() - << "\" reason=\"" << _detailedReason << "\" operations=\""; - - ost << getName() << "["; - for (uint32_t j = 0; j < getNodes().size(); j++) { - if (j != 0) { - ost << ","; - } - ost << getNodes()[j]; - } - ost << "]"; - - if (getStartTime().isSet()) { - uint64_t timeSpent( - (clock.getTimeInMillis() - getStartTime()).getTime()); - ost << "\" runtime_secs=\"" << timeSpent << "\""; - } else { - ost << "\""; - } - - ost << "/>"; - return ost.str(); -} - namespace { class IdealStateOpChecker : public PendingMessageTracker::Checker @@ -277,3 +248,5 @@ IdealStateOperation::shouldBlockThisOperation(uint32_t messageType, return false; } + +} diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h index 7906150d0cb..0e45d7f3b3a 100644 --- a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h +++ b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h @@ -208,8 +208,6 @@ public: */ void setCommandMeta(api::MaintenanceCommand& cmd) const; - std::string toXML(framework::Clock& clock) const; - std::string toString() const override; /** diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp index 481506096eb..27e203a9060 100644 --- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp @@ -1,5 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "mergeoperation.h" +#include <vespa/document/bucket/fixed_bucket_spaces.h> #include <vespa/storage/distributor/idealstatemanager.h> #include <vespa/storage/distributor/distributor_bucket_space.h> #include <vespa/storage/distributor/pendingmessagetracker.h> @@ -325,13 +326,30 @@ bool MergeOperation::shouldBlockThisOperation(uint32_t messageType, uint8_t pri) bool MergeOperation::isBlocked(const PendingMessageTracker& pending_tracker, const OperationSequencer& op_seq) const { - const auto& node_info = pending_tracker.getNodeInfo(); - for (auto node : getNodes()) { - if (node_info.isBusy(node)) { - return true; + // To avoid starvation of high priority global bucket merges, we do not consider + // these for blocking due to a node being "busy" (usually caused by a full merge + // throttler queue). + // + // This is for two reasons: + // 1. When an ideal state op is blocked, it is still removed from the internal + // maintenance priority queue. This means a blocked high pri operation will + // not be retried until the next DB pass (at which point the node is likely + // to still be marked as busy when there's heavy merge traffic). + // 2. Global bucket merges have high priority and will most likely be allowed + // to enter the merge throttler queues, displacing lower priority merges. + if (!is_global_bucket_merge()) { + const auto& node_info = pending_tracker.getNodeInfo(); + for (auto node : getNodes()) { + if (node_info.isBusy(node)) { + return true; + } } } return IdealStateOperation::isBlocked(pending_tracker, op_seq); } +bool MergeOperation::is_global_bucket_merge() const noexcept { + return getBucket().getBucketSpace() == document::FixedBucketSpaces::global_space(); +} + } diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h index 5df9421e815..11b5494fd9b 100644 --- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h +++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h @@ -61,6 +61,7 @@ private: void deleteSourceOnlyNodes(const BucketDatabase::Entry& currentState, DistributorStripeMessageSender& sender); + bool is_global_bucket_merge() const noexcept; }; } |