From 290bb392a1f7ec695ca9aa25a9f11935f0921150 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 2 Feb 2024 15:32:51 +0000 Subject: Condition probing has long been default --- .../src/tests/distributor/check_condition_test.cpp | 7 ---- .../tests/distributor/distributor_stripe_test.cpp | 20 ---------- .../distributor/distributor_stripe_test_util.cpp | 7 ---- storage/src/tests/distributor/putoperationtest.cpp | 2 - .../src/tests/distributor/removeoperationtest.cpp | 46 ++++++---------------- .../storage/config/distributorconfiguration.cpp | 2 - .../storage/config/distributorconfiguration.h | 8 ---- .../storage/config/stor-distributormanager.def | 5 --- .../operations/external/check_condition.cpp | 4 -- 9 files changed, 13 insertions(+), 88 deletions(-) diff --git a/storage/src/tests/distributor/check_condition_test.cpp b/storage/src/tests/distributor/check_condition_test.cpp index a47505a2ee6..5308d9a7c34 100644 --- a/storage/src/tests/distributor/check_condition_test.cpp +++ b/storage/src/tests/distributor/check_condition_test.cpp @@ -39,7 +39,6 @@ public: // By default, set up 2 nodes {0, 1} with mutually out of sync replica state // and with both reporting that they support condition probing. setup_stripe(2, 2, "version:1 storage:2 distributor:1"); - config_enable_condition_probing(true); tag_content_node_supports_condition_probing(0, true); tag_content_node_supports_condition_probing(1, true); addNodesToBucketDB(_bucket_id, "0=10/20/30/t,1=40/50/60"); @@ -119,12 +118,6 @@ public: CheckConditionTest::CheckConditionTest() = default; CheckConditionTest::~CheckConditionTest() = default; -TEST_F(CheckConditionTest, no_checker_returned_when_config_disabled) { - config_enable_condition_probing(false); - auto cond = create_check_condition(); - EXPECT_FALSE(cond); -} - TEST_F(CheckConditionTest, no_checker_returned_when_probing_not_supported_on_at_least_one_node) { tag_content_node_supports_condition_probing(1, false); auto cond = create_check_condition(); diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp index 2fb65c9d46d..b3efe8e4bad 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_enable_condition_probing(bool enable_probing) { - configure_stripe_with([&](auto& builder) { - builder.enableConditionProbing = enable_probing; - }); - } - void configure_enable_operation_cancellation(bool enable_cancellation) { configure_stripe_with([&](auto& builder) { builder.enableOperationCancellation = enable_cancellation; @@ -943,19 +937,6 @@ TEST_F(DistributorStripeTest, closing_aborts_gets_started_outside_stripe_thread) EXPECT_EQ(api::ReturnCode::ABORTED, _sender.reply(0)->getResult().getResult()); } -TEST_F(DistributorStripeTest, enable_condition_probing_config_is_propagated_to_internal_config) -{ - setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1"); - - EXPECT_TRUE(getConfig().enable_condition_probing()); - - configure_enable_condition_probing(false); - EXPECT_FALSE(getConfig().enable_condition_probing()); - - configure_enable_condition_probing(true); - EXPECT_TRUE(getConfig().enable_condition_probing()); -} - TEST_F(DistributorStripeTest, enable_operation_cancellation_config_is_propagated_to_internal_config) { setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1"); @@ -1023,7 +1004,6 @@ TEST_F(DistributorStripeTest, distribution_config_change_edge_cancels_pending_op void DistributorStripeTest::set_up_for_bucket_ownership_cancellation(uint32_t superbucket_idx) { setup_stripe(Redundancy(1), NodeCount(10), "version:1 distributor:2 storage:2"); configure_stripe_with([](auto& builder) { - builder.enableConditionProbing = true; builder.enableOperationCancellation = true; }); diff --git a/storage/src/tests/distributor/distributor_stripe_test_util.cpp b/storage/src/tests/distributor/distributor_stripe_test_util.cpp index 65bc6d1fe3e..923c7d1730b 100644 --- a/storage/src/tests/distributor/distributor_stripe_test_util.cpp +++ b/storage/src/tests/distributor/distributor_stripe_test_util.cpp @@ -553,13 +553,6 @@ DistributorStripeTestUtil::setSystemState(const lib::ClusterState& systemState) _stripe->enableClusterStateBundle(lib::ClusterStateBundle(systemState)); } -void -DistributorStripeTestUtil::config_enable_condition_probing(bool enable) { - auto cfg = make_config(); - cfg->set_enable_condition_probing(enable); - configure_stripe(cfg); -} - void DistributorStripeTestUtil::tag_content_node_supports_condition_probing(uint16_t index, bool supported) { NodeSupportedFeatures features; diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp index 5c6510ab1ec..f048472f3a9 100644 --- a/storage/src/tests/distributor/putoperationtest.cpp +++ b/storage/src/tests/distributor/putoperationtest.cpp @@ -724,7 +724,6 @@ TEST_F(PutOperationTest, minority_failure_override_not_in_effect_for_non_tas_err void PutOperationTest::set_up_tas_put_with_2_inconsistent_replica_nodes(bool create) { setup_stripe(Redundancy(2), NodeCount(2), "version:1 storage:2 distributor:1"); - config_enable_condition_probing(true); tag_content_node_supports_condition_probing(0, true); tag_content_node_supports_condition_probing(1, true); @@ -875,7 +874,6 @@ TEST_F(PutOperationTest, not_found_condition_probe_with_create_set_acts_as_if_ma TEST_F(PutOperationTest, conditional_put_no_replicas_case_with_create_set_acts_as_if_matched) { setup_stripe(Redundancy(2), NodeCount(2), "version:1 storage:2 distributor:1"); - config_enable_condition_probing(true); tag_content_node_supports_condition_probing(0, true); tag_content_node_supports_condition_probing(1, true); diff --git a/storage/src/tests/distributor/removeoperationtest.cpp b/storage/src/tests/distributor/removeoperationtest.cpp index 3da36312e18..f58634bfbae 100644 --- a/storage/src/tests/distributor/removeoperationtest.cpp +++ b/storage/src/tests/distributor/removeoperationtest.cpp @@ -105,7 +105,6 @@ struct ExtRemoveOperationTest : public RemoveOperationTest { void ExtRemoveOperationTest::set_up_tas_remove_with_2_nodes(ReplicaState replica_state) { setup_stripe(Redundancy(2), NodeCount(2), "version:1 storage:2 distributor:1"); - config_enable_condition_probing(true); tag_content_node_supports_condition_probing(0, true); tag_content_node_supports_condition_probing(1, true); @@ -134,8 +133,7 @@ TEST_F(RemoveOperationTest, simple) { sendRemove(); - ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 1", + ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 1", _sender.getLastCommand()); replyToMessage(*op, -1, 34); @@ -150,8 +148,7 @@ TEST_F(RemoveOperationTest, not_found) { sendRemove(); - ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 1", + ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 1", _sender.getLastCommand()); replyToMessage(*op, -1, 0); @@ -166,8 +163,7 @@ TEST_F(RemoveOperationTest, storage_failure) { sendRemove(); - ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 1", + ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 1", _sender.getLastCommand()); sendReply(*op, -1, api::ReturnCode::INTERNAL_FAILURE); @@ -190,12 +186,9 @@ TEST_F(RemoveOperationTest, multiple_copies) { sendRemove(); - ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 1," - "Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 2," - "Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 3", + ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 1," + "Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 2," + "Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 3", _sender.getCommands(true, true)); replyToMessage(*op, 0, 34); @@ -212,8 +205,7 @@ TEST_F(RemoveOperationTest, can_send_remove_when_all_replica_nodes_retired) { addNodesToBucketDB(bucketId, "0=123"); sendRemove(); - ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, " - "timestamp 100) => 0", + ASSERT_EQ("Remove(BucketId(0x4000000000000593), id:test:test::uri, timestamp 100) => 0", _sender.getLastCommand()); } @@ -231,9 +223,7 @@ TEST_F(ExtRemoveOperationTest, conditional_removes_are_instantly_successful_when ASSERT_NO_FATAL_FAILURE(set_up_tas_remove_with_2_nodes(ReplicaState::NONE)); ASSERT_EQ("", _sender.getCommands(true)); ASSERT_EQ(_sender.replies().size(), 1); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, not found) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, not found) " "ReturnCode(NONE)", _sender.getLastReply()); } @@ -257,9 +247,7 @@ TEST_F(ExtRemoveOperationTest, matching_condition_probe_sends_unconditional_remo ASSERT_TRUE(_sender.replies().empty()); reply_with(make_remove_reply(3, 50)); // remove from node 0 ASSERT_EQ(_sender.replies().size(), 1); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, removed doc from 50) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, removed doc from 50) " "ReturnCode(NONE)", _sender.getLastReply()); } @@ -271,9 +259,7 @@ TEST_F(ExtRemoveOperationTest, mismatching_condition_probe_fails_op_with_tas_err reply_with(make_get_reply(1, 50, false, false)); ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, not found) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, not found) " "ReturnCode(TEST_AND_SET_CONDITION_FAILED, Condition did not match document)", _sender.getLastReply()); } @@ -286,9 +272,7 @@ TEST_F(ExtRemoveOperationTest, not_found_condition_probe_fails_op_with_tas_error reply_with(make_get_reply(1, 0, false, false)); ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, not found) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, not found) " "ReturnCode(TEST_AND_SET_CONDITION_FAILED, Document does not exist)", _sender.getLastReply()); } @@ -300,9 +284,7 @@ TEST_F(ExtRemoveOperationTest, failed_condition_probe_fails_op_with_returned_err reply_with(make_failure_reply(1)); ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, not found) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, not found) " "ReturnCode(ABORTED, Failed during write repair condition probe step. Reason: " "One or more replicas failed during test-and-set condition evaluation)", _sender.getLastReply()); @@ -319,9 +301,7 @@ TEST_F(ExtRemoveOperationTest, cancellation_during_condition_probe_fails_operati reply_with(make_get_reply(1, 50, false, true)); ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); - EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), " - "id:test:test::uri, " - "timestamp 100, not found) " + EXPECT_EQ("RemoveReply(BucketId(0x0000000000000000), id:test:test::uri, timestamp 100, not found) " "ReturnCode(ABORTED, Failed during write repair condition probe step. Reason: " "Operation has been cancelled (likely due to a cluster state change))", _sender.getLastReply()); diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp index 7e22d2f7dba..8ac8c3e7414 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), - _enable_condition_probing(false), _enable_operation_cancellation(false), _minimumReplicaCountingMode(ReplicaCountingMode::TRUSTED) { @@ -151,7 +150,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; - _enable_condition_probing = config.enableConditionProbing; _enable_operation_cancellation = config.enableOperationCancellation; _minimumReplicaCountingMode = deriveReplicaCountingMode(config.minimumReplicaCountingMode); diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h index bbd0ed8ea45..07e247f7e44 100644 --- a/storage/src/vespa/storage/config/distributorconfiguration.h +++ b/storage/src/vespa/storage/config/distributorconfiguration.h @@ -231,13 +231,6 @@ public: return _max_activation_inhibited_out_of_sync_groups; } - void set_enable_condition_probing(bool enable) noexcept { - _enable_condition_probing = enable; - } - [[nodiscard]] bool enable_condition_probing() const noexcept { - return _enable_condition_probing; - } - [[nodiscard]] bool enable_operation_cancellation() const noexcept { return _enable_operation_cancellation; } @@ -282,7 +275,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 _enable_condition_probing; bool _enable_operation_cancellation; ReplicaCountingMode _minimumReplicaCountingMode; diff --git a/storage/src/vespa/storage/config/stor-distributormanager.def b/storage/src/vespa/storage/config/stor-distributormanager.def index c77d1b3eac3..7e0d557776c 100644 --- a/storage/src/vespa/storage/config/stor-distributormanager.def +++ b/storage/src/vespa/storage/config/stor-distributormanager.def @@ -174,11 +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 -## If true, a conditional Put or Remove operation received for a bucket with inconsistent -## replicas will trigger an implicit distributed condition probe to resolve the outcome of -## the condition across all divergent replicas. -enable_condition_probing bool default=true - ## If true, changes in the cluster where a subset of the nodes become unavailable or buckets ## change ownership between distributors will trigger an explicit cancellation of all pending ## requests partially or fully "invalidated" by such a change. diff --git a/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp b/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp index 76603df87c1..5437386d531 100644 --- a/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp @@ -246,10 +246,6 @@ CheckCondition::create_if_inconsistent_replicas(const document::Bucket& bucket, PersistenceOperationMetricSet& condition_probe_metrics, uint32_t trace_level) { - // TODO move this check to the caller? - if (!op_ctx.distributor_config().enable_condition_probing()) { - return {}; - } auto entries = get_bucket_database_entries(bucket_space, bucket.getBucketId()); if (entries.empty()) { return {}; // Not found -- cgit v1.2.3