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

This lets DeleteBucket operations FIFO with the client operations using
the default feed priority (120).

Not doing this risks preempting feed ops with deletes, elevating latencies.

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();