Merge pull request #20340 from vespa-engine/vekterli/cap-merge-delete-bucket-pri-to-default-feed-pri

Cap merge-induced DeleteBucket priority to that of default feed priority

2 files changed, 25 insertions, 8 deletions

diff --git a/storage/src/tests/distributor/mergeoperationtest.cpp b/storage/src/tests/distributor/mergeoperationtest.cpp
index e2b733c0a29..9e0c89819a7 100644
--- a/storage/src/tests/distributor/mergeoperationtest.cpp
+++ b/storage/src/tests/distributor/mergeoperationtest.cpp
@@ -25,6 +25,7 @@ vespalib::string _g_storage("storage");
 }
 
 struct MergeOperationTest : Test, DistributorStripeTestUtil {
+    using Priority = storage::api::StorageMessage::Priority;
     OperationSequencer _operation_sequencer;
 
     void SetUp() override {
@@ -38,8 +39,9 @@ struct MergeOperationTest : Test, DistributorStripeTestUtil {
     }
 
     std::shared_ptr<MergeOperation> setup_minimal_merge_op();
-    std::shared_ptr<MergeOperation> setup_simple_merge_op(const std::vector<uint16_t>& nodes);
-    std::shared_ptr<MergeOperation> setup_simple_merge_op();
+    std::shared_ptr<MergeOperation> setup_simple_merge_op(const std::vector<uint16_t>& nodes,
+                                                          Priority merge_pri = 120);
+    std::shared_ptr<MergeOperation> setup_simple_merge_op(Priority merge_pri = 120);
     void assert_simple_merge_bucket_command();
     void assert_simple_delete_bucket_command();
     MergeBucketMetricSet& get_merge_metrics();
@@ -55,7 +57,7 @@ MergeOperationTest::setup_minimal_merge_op()
 }
 
 std::shared_ptr<MergeOperation> 
-MergeOperationTest::setup_simple_merge_op(const std::vector<uint16_t>& nodes)
+MergeOperationTest::setup_simple_merge_op(const std::vector<uint16_t>& nodes, Priority merge_pri)
 {
     getClock().setAbsoluteTimeInSeconds(10);
 
@@ -68,15 +70,15 @@ MergeOperationTest::setup_simple_merge_op(const std::vector<uint16_t>& nodes)
 
     auto op = std::make_shared<MergeOperation>(BucketAndNodes(makeDocumentBucket(document::BucketId(16, 1)), nodes));
     op->setIdealStateManager(&getIdealStateManager());
-    op->setPriority(api::StorageMessage::Priority(125));
+    op->setPriority(merge_pri);
     op->start(_sender, framework::MilliSecTime(0));
     return op;
 }
 
 std::shared_ptr<MergeOperation>
-MergeOperationTest::setup_simple_merge_op()
+MergeOperationTest::setup_simple_merge_op(Priority merge_pri)
 {
-    return setup_simple_merge_op({0, 1, 2});
+    return setup_simple_merge_op({0, 1, 2}, merge_pri);
 }
 
 void
@@ -605,13 +607,25 @@ TEST_F(MergeOperationTest, unordered_merges_only_sent_iff_config_enabled_and_all
 }
 
 TEST_F(MergeOperationTest, delete_bucket_inherits_merge_priority) {
-    auto op = setup_simple_merge_op();
+    auto op = setup_simple_merge_op(Priority(125));
     ASSERT_NO_FATAL_FAILURE(assert_simple_merge_bucket_command());
     sendReply(*op);
     ASSERT_NO_FATAL_FAILURE(assert_simple_delete_bucket_command());
     auto del_cmd = std::dynamic_pointer_cast<api::DeleteBucketCommand>(_sender.commands().back());
     ASSERT_TRUE(del_cmd);
     EXPECT_EQ(int(del_cmd->getPriority()), int(op->getPriority()));
+    EXPECT_EQ(int(del_cmd->getPriority()), 125);
+}
+
+// TODO less magical numbers, but the priority mapping is technically config...
+TEST_F(MergeOperationTest, delete_bucket_priority_is_capped_to_feed_pri_120) {
+    auto op = setup_simple_merge_op(Priority(119));
+    ASSERT_NO_FATAL_FAILURE(assert_simple_merge_bucket_command());
+    sendReply(*op);
+    ASSERT_NO_FATAL_FAILURE(assert_simple_delete_bucket_command());
+    auto del_cmd = std::dynamic_pointer_cast<api::DeleteBucketCommand>(_sender.commands().back());
+    ASSERT_TRUE(del_cmd);
+    EXPECT_EQ(int(del_cmd->getPriority()), 120);
 }
 
 } // storage::distributor
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
index 6624ca7c8c6..6aa243d5e99 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
@@ -253,7 +253,10 @@ MergeOperation::deleteSourceOnlyNodes(
             return;
         }
         _removeOperation->setIdealStateManager(_manager);
-        _removeOperation->setPriority(getPriority());
+        // We cap the DeleteBucket pri so that it FIFOs with the default feed priority (120).
+        // Not doing this risks preempting feed ops with deletes, elevating latencies.
+        // TODO less magical numbers, but the priority mapping is technically config...
+        _removeOperation->setPriority(std::max(api::StorageMessage::Priority(120), getPriority()));
         
         if (_removeOperation->onStartInternal(sender)) {
             _ok = _removeOperation->ok();