- Hardcode enable_metadata_only_fetch_phase_for_inconsistent_updates and restart_with_fast_update_path_if_all_get_timestamps_are_consistent to true.

- The tests expecting depending on these flags specify these values explicit.

7 files changed, 43 insertions, 71 deletions

diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp
index b3efe8e4bad..d866cffdcb9 100644
--- a/storage/src/tests/distributor/distributor_stripe_test.cpp
+++ b/storage/src/tests/distributor/distributor_stripe_test.cpp
@@ -144,10 +144,8 @@ struct DistributorStripeTest : Test, DistributorStripeTestUtil {
         });
     }
 
-    void configure_update_fast_path_restart_enabled(bool enabled) {
-        configure_stripe_with([&](auto& builder) {
-            builder.restartWithFastUpdatePathIfAllGetTimestampsAreConsistent = enabled;
-        });
+    void configure_update_fast_path_restart_enabled() {
+        configure_stripe_with([&](auto& builder) { (void) builder; });
     }
 
     void configure_merge_operations_disabled(bool disabled) {
@@ -162,10 +160,8 @@ struct DistributorStripeTest : Test, DistributorStripeTestUtil {
         });
     }
 
-    void configure_metadata_update_phase_enabled(bool enabled) {
-        configure_stripe_with([&](auto& builder) {
-            builder.enableMetadataOnlyFetchPhaseForInconsistentUpdates = enabled;
-        });
+    void configure_metadata_update_phase_enabled() {
+        configure_stripe_with([&](auto& builder) { (void) builder; });
     }
 
     void configure_max_activation_inhibited_out_of_sync_groups(uint32_t n_groups) {
@@ -810,12 +806,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 +822,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 7e0d557776c..927fe1db73b 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();