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 From 2b5ad6833695e104e45354c5cd726471c15a99a9 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 2 Feb 2024 14:04:15 +0000 Subject: Only include what you need --- storage/src/vespa/storage/bucketdb/bucketdatabase.h | 1 - storage/src/vespa/storage/config/distributorconfiguration.cpp | 1 + storage/src/vespa/storage/config/distributorconfiguration.h | 3 ++- .../storage/distributor/distributor_stripe_operation_context.h | 9 +++++---- 4 files changed, 8 insertions(+), 6 deletions(-) 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 #include -#include #include namespace storage { diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp index e58e366cea9..018cafa3fb8 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 #include #include #include diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h index f259bfd5b52..2b9dc3eefda 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 #include 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; 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 -#include -#include #include -namespace document { class Bucket; } +namespace document { + class Bucket; + class DocumentId; +} namespace storage::lib { class ClusterStateBundle; } - +namespace storage { class DistributorConfiguration; } namespace storage::distributor { class PendingMessageTracker; -- cgit v1.2.3 From da289e783f440d9c6df2fbc29af8ad2a5b2bcae2 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 2 Feb 2024 14:52:37 +0000 Subject: Keep priority_merge_out_of_sync_copies until it can be safely cleaned out. --- storage/src/vespa/storage/config/stor-distributormanager.def | 3 +++ 1 file changed, 3 insertions(+) diff --git a/storage/src/vespa/storage/config/stor-distributormanager.def b/storage/src/vespa/storage/config/stor-distributormanager.def index ffeb858a5ff..f6e88a1edc8 100644 --- a/storage/src/vespa/storage/config/stor-distributormanager.def +++ b/storage/src/vespa/storage/config/stor-distributormanager.def @@ -194,3 +194,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 -- cgit v1.2.3