Merge pull request #30146 from vespa-engine/balder/always-unordered-merging

Balder/always unordered merging

8 files changed, 17 insertions, 63 deletions

diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp
index fb67ccde3ba..4568447a28d 100644
--- a/storage/src/tests/distributor/distributor_stripe_test.cpp
+++ b/storage/src/tests/distributor/distributor_stripe_test.cpp
@@ -174,12 +174,6 @@ struct DistributorStripeTest : Test, DistributorStripeTestUtil {
         });
     }
 
-    void configure_use_unordered_merge_chaining(bool use_unordered) {
-        configure_stripe_with([&](auto& builder) {
-            builder.useUnorderedMergeChaining = use_unordered;
-        });
-    }
-
     void configure_enable_two_phase_garbage_collection(bool use_two_phase) {
         configure_stripe_with([&](auto& builder) {
             builder.enableTwoPhaseGarbageCollection = use_two_phase;
@@ -955,17 +949,6 @@ TEST_F(DistributorStripeTest, closing_aborts_gets_started_outside_stripe_thread)
     EXPECT_EQ(api::ReturnCode::ABORTED, _sender.reply(0)->getResult().getResult());
 }
 
-TEST_F(DistributorStripeTest, use_unordered_merge_chaining_config_is_propagated_to_internal_config)
-{
-    setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1");
-
-    configure_use_unordered_merge_chaining(true);
-    EXPECT_TRUE(getConfig().use_unordered_merge_chaining());
-
-    configure_use_unordered_merge_chaining(false);
-    EXPECT_FALSE(getConfig().use_unordered_merge_chaining());
-}
-
 TEST_F(DistributorStripeTest, enable_two_phase_gc_config_is_propagated_to_internal_config)
 {
     setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1");
diff --git a/storage/src/tests/distributor/mergeoperationtest.cpp b/storage/src/tests/distributor/mergeoperationtest.cpp
index 8c623eb7ba4..754974ebf97 100644
--- a/storage/src/tests/distributor/mergeoperationtest.cpp
+++ b/storage/src/tests/distributor/mergeoperationtest.cpp
@@ -5,7 +5,6 @@
 #include <vespa/document/test/make_document_bucket.h>
 #include <vespa/storage/distributor/top_level_bucket_db_updater.h>
 #include <vespa/storage/distributor/top_level_distributor.h>
-#include <vespa/storage/distributor/idealstatemanager.h>
 #include <vespa/storage/distributor/operation_sequencer.h>
 #include <vespa/storage/distributor/operations/idealstate/mergeoperation.h>
 #include <vespa/storage/config/distributorconfiguration.h>
@@ -574,9 +573,7 @@ TEST_F(MergeOperationTest, unordered_merges_only_sent_iff_config_enabled_and_all
     set_node_supported_features(1, with_unordered);
     set_node_supported_features(2, with_unordered);
 
-    auto config = make_config();
-    config->set_use_unordered_merge_chaining(true);
-    configure_stripe(std::move(config));
+    configure_stripe(make_config());
 
     // Only nodes {1, 2} support unordered merging; merges should be ordered (sent to lowest index node 1).
     setup_simple_merge_op({1, 2, 3}); // Note: these will be re-ordered in ideal state order internally
@@ -593,17 +590,6 @@ TEST_F(MergeOperationTest, unordered_merges_only_sent_iff_config_enabled_and_all
               _sender.getLastCommand(true));
 
     _sender.clear();
-
-    config = make_config();
-    config->set_use_unordered_merge_chaining(false);
-    configure_stripe(std::move(config));
-
-    // If config is not enabled, should send ordered even if nodes support the feature.
-    setup_simple_merge_op({2, 1});
-    ASSERT_EQ("MergeBucketCommand(BucketId(0x4000000000000001), to time 10000002, "
-              "cluster state version: 0, nodes: [2, 1], chain: [], "
-              "estimated memory footprint: 2 bytes, reasons to start: ) => 1",
-              _sender.getLastCommand(true));
 }
 
 TEST_F(MergeOperationTest, delete_bucket_inherits_merge_priority) {
diff --git a/storage/src/vespa/storage/bucketdb/bucketdatabase.h b/storage/src/vespa/storage/bucketdb/bucketdatabase.h
index 4ad52697ac1..5fed99cb801 100644
--- a/storage/src/vespa/storage/bucketdb/bucketdatabase.h
+++ b/storage/src/vespa/storage/bucketdb/bucketdatabase.h
@@ -8,7 +8,6 @@
 #include "read_guard.h"
 #include <vespa/vespalib/util/printable.h>
 #include <vespa/storage/bucketdb/bucketinfo.h>
-#include <vespa/document/bucket/bucketid.h>
 #include <vespa/vespalib/util/memoryusage.h>
 
 namespace storage {
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp
index a06ff241ee3..ff4f4a7e7bd 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.cpp
+++ b/storage/src/vespa/storage/config/distributorconfiguration.cpp
@@ -1,5 +1,6 @@
 // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #include "distributorconfiguration.h"
+#include <vespa/storage/common/storagecomponent.h>
 #include <vespa/storage/config/config-stor-distributormanager.h>
 #include <vespa/storage/config/config-stor-visitordispatcher.h>
 #include <vespa/document/select/parser.h>
@@ -44,7 +45,6 @@ DistributorConfiguration::DistributorConfiguration(StorageComponent& component)
       _merge_operations_disabled(false),
       _use_weak_internal_read_consistency_for_client_gets(false),
       _enable_metadata_only_fetch_phase_for_inconsistent_updates(false),
-      _use_unordered_merge_chaining(false),
       _enable_two_phase_garbage_collection(false),
       _enable_condition_probing(false),
       _enable_operation_cancellation(false),
@@ -152,7 +152,6 @@ DistributorConfiguration::configure(const DistributorManagerConfig & config)
     _use_weak_internal_read_consistency_for_client_gets = config.useWeakInternalReadConsistencyForClientGets;
     _enable_metadata_only_fetch_phase_for_inconsistent_updates = config.enableMetadataOnlyFetchPhaseForInconsistentUpdates;
     _max_activation_inhibited_out_of_sync_groups = config.maxActivationInhibitedOutOfSyncGroups;
-    _use_unordered_merge_chaining = config.useUnorderedMergeChaining;
     _enable_two_phase_garbage_collection = config.enableTwoPhaseGarbageCollection;
     _enable_condition_probing = config.enableConditionProbing;
     _enable_operation_cancellation = config.enableOperationCancellation;
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h
index 2b78d627b4d..8b161103747 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.h
+++ b/storage/src/vespa/storage/config/distributorconfiguration.h
@@ -2,7 +2,6 @@
 #pragma once
 
 #include "replica_counting_mode.h"
-#include <vespa/storage/common/storagecomponent.h>
 #include <vespa/vespalib/util/time.h>
 
 namespace vespa::config::content::core::internal {
@@ -12,6 +11,8 @@ namespace vespa::config::content::core::internal {
 
 namespace storage {
 
+class StorageComponent;
+
 class DistributorConfiguration {
 public:
     DistributorConfiguration(const DistributorConfiguration& other) = delete;
@@ -230,13 +231,6 @@ public:
         return _max_activation_inhibited_out_of_sync_groups;
     }
 
-    void set_use_unordered_merge_chaining(bool unordered) noexcept {
-        _use_unordered_merge_chaining = unordered;
-    }
-    [[nodiscard]] bool use_unordered_merge_chaining() const noexcept {
-        return _use_unordered_merge_chaining;
-    }
-
     void set_enable_two_phase_garbage_collection(bool enable) noexcept {
         _enable_two_phase_garbage_collection = enable;
     }
@@ -294,7 +288,6 @@ private:
     bool _merge_operations_disabled;
     bool _use_weak_internal_read_consistency_for_client_gets;
     bool _enable_metadata_only_fetch_phase_for_inconsistent_updates;
-    bool _use_unordered_merge_chaining;
     bool _enable_two_phase_garbage_collection;
     bool _enable_condition_probing;
     bool _enable_operation_cancellation;
diff --git a/storage/src/vespa/storage/config/stor-distributormanager.def b/storage/src/vespa/storage/config/stor-distributormanager.def
index 1260350630e..48316c0187f 100644
--- a/storage/src/vespa/storage/config/stor-distributormanager.def
+++ b/storage/src/vespa/storage/config/stor-distributormanager.def
@@ -174,13 +174,6 @@ max_activation_inhibited_out_of_sync_groups int default=0
 ## Stripe counts must be a power of two.
 num_distributor_stripes int default=0 restart
 
-## Enables sending merges that are forwarded between content nodes in ideal state node key
-## order, instead of strictly increasing node key order (which is the default).
-## Even if this config is set to true, unordered merges will only be sent if _all_ nodes
-## involved in a given merge have previously reported (as part of bucket info fetching)
-## that they support the unordered merge feature.
-use_unordered_merge_chaining bool default=true
-
 ## If true, garbage collection is performed in two phases (metadata gathering and deletion)
 ## instead of just a single phase. Two-phase GC allows for ensuring the same set of documents
 ## is deleted across all nodes and explicitly takes write locks on the distributor to prevent
@@ -198,3 +191,6 @@ enable_condition_probing bool default=true
 ## change ownership between distributors will trigger an explicit cancellation of all pending
 ## requests partially or fully "invalidated" by such a change.
 enable_operation_cancellation bool default=false
+
+## TODO GC very soon, it has no effect.
+priority_merge_out_of_sync_copies int default=120
diff --git a/storage/src/vespa/storage/distributor/distributor_stripe_operation_context.h b/storage/src/vespa/storage/distributor/distributor_stripe_operation_context.h
index e9c405ecff3..2109c813014 100644
--- a/storage/src/vespa/storage/distributor/distributor_stripe_operation_context.h
+++ b/storage/src/vespa/storage/distributor/distributor_stripe_operation_context.h
@@ -6,13 +6,14 @@
 #include "bucketownership.h"
 #include "operation_routing_snapshot.h"
 #include <vespa/document/bucket/bucketspace.h>
-#include <vespa/storage/bucketdb/bucketdatabase.h>
-#include <vespa/storage/common/distributorcomponent.h>
 #include <vespa/storageapi/defs.h>
 
-namespace document { class Bucket; }
+namespace document {
+    class Bucket;
+    class DocumentId;
+}
 namespace storage::lib { class ClusterStateBundle; }
-
+namespace storage { class DistributorConfiguration; }
 namespace storage::distributor {
 
 class PendingMessageTracker;
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
index 0eece641816..2edcd5e2597 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
@@ -147,8 +147,7 @@ MergeOperation::onStart(DistributorStripeMessageSender& sender)
         auto msg = std::make_shared<api::MergeBucketCommand>(getBucket(), _mnodes,
                                                              _manager->operation_context().generate_unique_timestamp(),
                                                              clusterState.getVersion());
-        const bool may_send_unordered = (_manager->operation_context().distributor_config().use_unordered_merge_chaining()
-                                         && all_involved_nodes_support_unordered_merge_chaining());
+        const bool may_send_unordered = all_involved_nodes_support_unordered_merge_chaining();
         if (!may_send_unordered) {
             // Due to merge forwarding/chaining semantics, we must always send
             // the merge command to the lowest indexed storage node involved in
@@ -364,12 +363,10 @@ bool MergeOperation::is_global_bucket_merge() const noexcept {
 
 bool MergeOperation::all_involved_nodes_support_unordered_merge_chaining() const noexcept {
     const auto& features_repo = _manager->operation_context().node_supported_features_repo();
-    for (uint16_t node : getNodes()) {
-        if (!features_repo.node_supported_features(node).unordered_merge_chaining) {
-            return false;
-        }
-    }
-    return true;
+    const auto & nodes = getNodes();
+    return std::all_of(nodes.begin(), nodes.end(), [&features_repo](uint16_t node) {
+        return features_repo.node_supported_features(node).unordered_merge_chaining;
+    });
 }
 
 uint32_t MergeOperation::estimate_merge_memory_footprint_upper_bound(const std::vector<MergeMetaData>& nodes) const noexcept {