From 1b36ead1a807e8d1fef299915aa8a8afac436fe0 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 2 Feb 2024 13:52:23 +0000 Subject: Alwasy use use_unordered_merge_chaining --- .../src/tests/distributor/distributor_stripe_test.cpp | 17 ----------------- storage/src/tests/distributor/mergeoperationtest.cpp | 16 +--------------- .../vespa/storage/config/distributorconfiguration.cpp | 2 -- .../src/vespa/storage/config/distributorconfiguration.h | 8 -------- .../vespa/storage/config/stor-distributormanager.def | 7 ------- .../operations/idealstate/mergeoperation.cpp | 13 +++++-------- 6 files changed, 6 insertions(+), 57 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 #include #include -#include #include #include #include @@ -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/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp index e386ac3583c..e58e366cea9 100644 --- a/storage/src/vespa/storage/config/distributorconfiguration.cpp +++ b/storage/src/vespa/storage/config/distributorconfiguration.cpp @@ -45,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), @@ -154,7 +153,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 4f561f7df5a..f259bfd5b52 100644 --- a/storage/src/vespa/storage/config/distributorconfiguration.h +++ b/storage/src/vespa/storage/config/distributorconfiguration.h @@ -230,13 +230,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; } @@ -295,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 06faf96a286..ffeb858a5ff 100644 --- a/storage/src/vespa/storage/config/stor-distributormanager.def +++ b/storage/src/vespa/storage/config/stor-distributormanager.def @@ -177,13 +177,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 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(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& nodes) const noexcept { -- cgit v1.2.3