diff options
7 files changed, 39 insertions, 75 deletions
diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp index b3efe8e4bad..fbc9cb44462 100644 --- a/storage/src/tests/distributor/distributor_stripe_test.cpp +++ b/storage/src/tests/distributor/distributor_stripe_test.cpp @@ -144,12 +144,6 @@ struct DistributorStripeTest : Test, DistributorStripeTestUtil { }); } - void configure_update_fast_path_restart_enabled(bool enabled) { - configure_stripe_with([&](auto& builder) { - builder.restartWithFastUpdatePathIfAllGetTimestampsAreConsistent = enabled; - }); - } - void configure_merge_operations_disabled(bool disabled) { configure_stripe_with([&](auto& builder) { builder.mergeOperationsDisabled = disabled; @@ -162,12 +156,6 @@ struct DistributorStripeTest : Test, DistributorStripeTestUtil { }); } - void configure_metadata_update_phase_enabled(bool enabled) { - configure_stripe_with([&](auto& builder) { - builder.enableMetadataOnlyFetchPhaseForInconsistentUpdates = enabled; - }); - } - void configure_max_activation_inhibited_out_of_sync_groups(uint32_t n_groups) { configure_stripe_with([&](auto& builder) { builder.maxActivationInhibitedOutOfSyncGroups = n_groups; @@ -810,12 +798,6 @@ TEST_F(DistributorStripeTest, fast_path_on_consistent_gets_config_is_propagated_ setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1"); EXPECT_TRUE(getConfig().update_fast_path_restart_enabled()); // Enabled by default - - configure_update_fast_path_restart_enabled(true); - EXPECT_TRUE(getConfig().update_fast_path_restart_enabled()); - - configure_update_fast_path_restart_enabled(false); - EXPECT_FALSE(getConfig().update_fast_path_restart_enabled()); } TEST_F(DistributorStripeTest, merge_disabling_config_is_propagated_to_internal_config) @@ -832,12 +814,7 @@ TEST_F(DistributorStripeTest, merge_disabling_config_is_propagated_to_internal_c TEST_F(DistributorStripeTest, metadata_update_phase_config_is_propagated_to_internal_config) { setup_stripe(Redundancy(1), NodeCount(1), "distributor:1 storage:1"); - - configure_metadata_update_phase_enabled(true); EXPECT_TRUE(getConfig().enable_metadata_only_fetch_phase_for_inconsistent_updates()); - - configure_metadata_update_phase_enabled(false); - EXPECT_FALSE(getConfig().enable_metadata_only_fetch_phase_for_inconsistent_updates()); } TEST_F(DistributorStripeTest, weak_internal_read_consistency_config_is_propagated_to_internal_configs) diff --git a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp index c9dfa346424..8ccca37e6c1 100644 --- a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp +++ b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp @@ -173,6 +173,20 @@ struct TwoPhaseUpdateOperationTest : Test, DistributorStripeTestUtil { {}, {true, "full disk"}, false)); } + static std::shared_ptr<DistributorConfiguration> + with_fast_path_restart(std::shared_ptr<DistributorConfiguration> cfg, bool value) + { + cfg->set_update_fast_path_restart_enabled(value); + return cfg; + } + + static std::shared_ptr<DistributorConfiguration> + with_meta_only_fetch(std::shared_ptr<DistributorConfiguration> cfg, bool value) + { + cfg->set_enable_metadata_only_fetch_phase_for_inconsistent_updates(value); + return cfg; + } + }; TwoPhaseUpdateOperationTest::TwoPhaseUpdateOperationTest() = default; @@ -674,6 +688,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_updates_newest_received_document) TEST_F(TwoPhaseUpdateOperationTest, create_if_non_existent_creates_document_if_all_empty_gets) { setup_stripe(3, 3, "storage:3 distributor:1"); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=1/2/3,2=2/3/4", UpdateOptions().createIfNonExistent(true)); cb->start(_sender); @@ -705,6 +720,7 @@ TEST_F(TwoPhaseUpdateOperationTest, create_if_non_existent_creates_document_if_a TEST_F(TwoPhaseUpdateOperationTest, update_fails_if_safe_path_has_failed_put) { setup_stripe(3, 3, "storage:3 distributor:1"); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=1/2/3,2=2/3/4", UpdateOptions().createIfNonExistent(true)); cb->start(_sender); @@ -990,6 +1006,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_parse_failure_fails_with TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_unknown_doc_type_fails_with_illegal_params_error) { setup_stripe(2, 2, "storage:2 distributor:1"); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=2/3/4", UpdateOptions().condition("langbein.headerval=1234")); cb->start(_sender); @@ -1009,6 +1026,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_unknown_doc_type_fails_w TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_with_missing_doc_and_no_auto_create_fails_with_tas_error) { setup_stripe(2, 2, "storage:2 distributor:1"); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=2/3/4", UpdateOptions().condition("testdoctype1.headerval==120")); cb->start(_sender); @@ -1028,6 +1046,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_with_missing_doc_and_no_ TEST_F(TwoPhaseUpdateOperationTest, safe_path_condition_with_missing_doc_and_auto_create_sends_puts) { setup_stripe(2, 2, "storage:2 distributor:1"); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=2/3/4", UpdateOptions() .condition("testdoctype1.headerval==120") .createIfNonExistent(true)); @@ -1100,9 +1119,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_close_edge_sends_correct_reply) { TEST_F(TwoPhaseUpdateOperationTest, safe_path_consistent_get_reply_timestamps_restarts_with_fast_path_if_enabled) { setup_stripe(2, 2, "storage:2 distributor:1"); - auto cfg = make_config(); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1128,9 +1145,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_consistent_get_reply_timestamps_re TEST_F(TwoPhaseUpdateOperationTest, safe_path_consistent_get_reply_timestamps_does_not_restart_with_fast_path_if_disabled) { setup_stripe(2, 2, "storage:2 distributor:1"); - auto cfg = make_config(); - cfg->set_update_fast_path_restart_enabled(false); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), false)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1149,9 +1164,7 @@ TEST_F(TwoPhaseUpdateOperationTest, safe_path_consistent_get_reply_timestamps_do TEST_F(TwoPhaseUpdateOperationTest, fast_path_not_restarted_if_replica_set_altered_between_get_send_and_receive) { setup_stripe(3, 3, "storage:3 distributor:1"); - auto cfg = make_config(); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1175,9 +1188,7 @@ TEST_F(TwoPhaseUpdateOperationTest, fast_path_not_restarted_if_replica_set_alter TEST_F(TwoPhaseUpdateOperationTest, fast_path_not_restarted_if_document_not_found_on_a_replica_node) { setup_stripe(2, 2, "storage:2 distributor:1"); - auto cfg = make_config(); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(with_meta_only_fetch(make_config(), false), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1193,9 +1204,7 @@ TEST_F(TwoPhaseUpdateOperationTest, fast_path_not_restarted_if_document_not_foun // Buckets must be created from scratch by Put operations, updates alone cannot do this. TEST_F(TwoPhaseUpdateOperationTest, fast_path_not_restarted_if_no_initial_replicas_exist) { setup_stripe(2, 2, "storage:2 distributor:1"); - auto cfg = make_config(); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(make_config(), false)); // No replicas, technically consistent but cannot use fast path. auto cb = sendUpdate("", UpdateOptions().createIfNonExistent(true)); @@ -1211,7 +1220,7 @@ TEST_F(TwoPhaseUpdateOperationTest, update_gets_are_sent_with_strong_consistency setup_stripe(2, 2, "storage:2 distributor:1"); auto cfg = make_config(); cfg->set_use_weak_internal_read_consistency_for_client_gets(true); - configure_stripe(cfg); + configure_stripe(with_meta_only_fetch(std::move(cfg), false)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1416,8 +1425,7 @@ TEST_F(ThreePhaseUpdateTest, single_full_get_cannot_restart_in_fast_path) { setup_stripe(2, 2, "storage:2 distributor:1"); auto cfg = make_config(); cfg->set_enable_metadata_only_fetch_phase_for_inconsistent_updates(true); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(std::move(cfg), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas. cb->start(_sender); @@ -1536,8 +1544,7 @@ TEST_F(ThreePhaseUpdateTest, single_full_get_tombstone_is_no_op_without_auto_cre setup_stripe(2, 2, "storage:2 distributor:1"); auto cfg = make_config(); cfg->set_enable_metadata_only_fetch_phase_for_inconsistent_updates(true); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(std::move(cfg), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4"); cb->start(_sender); @@ -1560,8 +1567,7 @@ TEST_F(ThreePhaseUpdateTest, single_full_get_tombstone_sends_puts_with_auto_crea setup_stripe(2, 2, "storage:2 distributor:1"); auto cfg = make_config(); cfg->set_enable_metadata_only_fetch_phase_for_inconsistent_updates(true); - cfg->set_update_fast_path_restart_enabled(true); - configure_stripe(cfg); + configure_stripe(with_fast_path_restart(std::move(cfg), true)); auto cb = sendUpdate("0=1/2/3,1=2/3/4", UpdateOptions().createIfNonExistent(true)); cb->start(_sender); diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp index 8ac8c3e7414..6957e541d6b 100644 --- a/storage/src/vespa/storage/config/distributorconfiguration.cpp +++ b/storage/src/vespa/storage/config/distributorconfiguration.cpp @@ -41,10 +41,10 @@ DistributorConfiguration::DistributorConfiguration(StorageComponent& component) _enableInconsistentJoin(false), _disableBucketActivation(false), _allowStaleReadsDuringClusterStateTransitions(false), - _update_fast_path_restart_enabled(false), + _update_fast_path_restart_enabled(true), _merge_operations_disabled(false), _use_weak_internal_read_consistency_for_client_gets(false), - _enable_metadata_only_fetch_phase_for_inconsistent_updates(false), + _enable_metadata_only_fetch_phase_for_inconsistent_updates(true), _enable_operation_cancellation(false), _minimumReplicaCountingMode(ReplicaCountingMode::TRUSTED) { @@ -145,10 +145,8 @@ DistributorConfiguration::configure(const DistributorManagerConfig & config) _disableBucketActivation = config.disableBucketActivation; _allowStaleReadsDuringClusterStateTransitions = config.allowStaleReadsDuringClusterStateTransitions; - _update_fast_path_restart_enabled = config.restartWithFastUpdatePathIfAllGetTimestampsAreConsistent; _merge_operations_disabled = config.mergeOperationsDisabled; _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_operation_cancellation = config.enableOperationCancellation; _minimumReplicaCountingMode = deriveReplicaCountingMode(config.minimumReplicaCountingMode); @@ -162,6 +160,10 @@ DistributorConfiguration::configure(const DistributorManagerConfig & config) _simulated_db_pruning_latency = std::chrono::milliseconds(std::max(0, config.simulatedDbPruningLatencyMsec)); _simulated_db_merging_latency = std::chrono::milliseconds(std::max(0, config.simulatedDbMergingLatencyMsec)); + // TODO GC + _update_fast_path_restart_enabled = true; + _enable_metadata_only_fetch_phase_for_inconsistent_updates = true; + LOG(debug, "Distributor now using new configuration parameters. Split limits: %d docs/%d bytes. " "Join limits: %d docs/%d bytes. Minimal bucket split %d. " diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h index 07e247f7e44..38fac13150c 100644 --- a/storage/src/vespa/storage/config/distributorconfiguration.h +++ b/storage/src/vespa/storage/config/distributorconfiguration.h @@ -271,10 +271,10 @@ private: bool _enableInconsistentJoin; bool _disableBucketActivation; bool _allowStaleReadsDuringClusterStateTransitions; - bool _update_fast_path_restart_enabled; + bool _update_fast_path_restart_enabled; //TODO Rewrite tests and GC bool _merge_operations_disabled; bool _use_weak_internal_read_consistency_for_client_gets; - bool _enable_metadata_only_fetch_phase_for_inconsistent_updates; + bool _enable_metadata_only_fetch_phase_for_inconsistent_updates; //TODO Rewrite tests and GC 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 a880ab6f00d..3f6028d7fa1 100644 --- a/storage/src/vespa/storage/config/stor-distributormanager.def +++ b/storage/src/vespa/storage/config/stor-distributormanager.def @@ -112,17 +112,6 @@ allow_stale_reads_during_cluster_state_transitions bool default=false simulated_db_pruning_latency_msec int default=0 simulated_db_merging_latency_msec int default=0 -## If a bucket is inconsistent and an Update operation is received, a two-phase -## write-repair path is triggered in which a Get is sent to all diverging replicas. -## Once received, the update is applied on the distributor and pushed out to the -## content nodes as Puts. -## Iff this config is set to true AND all Gets return the same timestamp from all -## content nodes, the two-phase update path reverts back to the regular fast path. -## Since all replicas of the document were in sync, applying the update in-place -## shall be considered safe. -## DEPRECATED -- always enabled with 3-phase updates. -restart_with_fast_update_path_if_all_get_timestamps_are_consistent bool default=true - ## If set, no merge operations may be generated for any reason by a distributor. ## This is ONLY intended for system testing of certain transient edge cases and ## MUST NOT be set to true in a production environment. @@ -137,15 +126,6 @@ merge_operations_disabled bool default=false ## This is mostly useful in a system that is effectively read-only. use_weak_internal_read_consistency_for_client_gets bool default=false -## If true, adds an initial metadata-only fetch phase to updates that touch buckets -## with inconsistent replicas. Metadata timestamps are compared and a single full Get -## is sent _only_ to one node with the highest timestamp. Without a metadata phase, -## full gets would be sent to _all_ nodes. -## Setting this option to true always implicitly enables the fast update restart -## feature, so it's not required to set that config to true, nor will setting it -## to false actually disable the feature. -enable_metadata_only_fetch_phase_for_inconsistent_updates bool default=true - ## If a distributor main thread tick is constantly processing requests or responses ## originating from other nodes, setting this value above zero will prevent implicit ## maintenance scans from being done as part of the tick for up to N rounds of ticking. diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp index ef004fc34b8..84e9ab71bcb 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp @@ -93,9 +93,8 @@ TwoPhaseUpdateOperation::ensureUpdateReplyCreated() } void -TwoPhaseUpdateOperation::sendReply( - DistributorStripeMessageSender& sender, - std::shared_ptr<api::UpdateReply> reply) +TwoPhaseUpdateOperation::sendReply(DistributorStripeMessageSender& sender, + const std::shared_ptr<api::UpdateReply> & reply) { assert(!_replySent); reply->getTrace().addChild(std::move(_trace)); diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h index b12869225e6..0511c07a5c1 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h @@ -92,7 +92,7 @@ private: static const char* stateToString(SendState) noexcept; void sendReply(DistributorStripeMessageSender&, - std::shared_ptr<api::UpdateReply>); + const std::shared_ptr<api::UpdateReply> &); void sendReplyWithResult(DistributorStripeMessageSender&, const api::ReturnCode&); void ensureUpdateReplyCreated(); |