Add comments and some extra safety handling of single Get command

4 files changed, 26 insertions, 2 deletions

diff --git a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp
index 82466ec5ed7..f3ce4d92263 100644
--- a/storage/src/tests/distributor/twophaseupdateoperationtest.cpp
+++ b/storage/src/tests/distributor/twophaseupdateoperationtest.cpp
@@ -142,7 +142,7 @@ struct TwoPhaseUpdateOperationTest : Test, DistributorTestUtil {
     std::shared_ptr<TwoPhaseUpdateOperation> set_up_2_inconsistent_replicas_and_start_update(bool enable_3phase = true) {
         setupDistributor(2, 2, "storage:2 distributor:1");
         getConfig().set_enable_metadata_only_fetch_phase_for_inconsistent_updates(enable_3phase);
-        std::shared_ptr<TwoPhaseUpdateOperation> cb(sendUpdate("0=1/2/3,1=2/3/4")); // Inconsistent replicas.
+        auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas.
         cb->start(_sender, framework::MilliSecTime(0));
         return cb;
     }
@@ -1194,6 +1194,21 @@ TEST_F(ThreePhaseUpdateTest, update_failed_with_transient_error_code_if_replica_
               _sender.getLastReply(true));
 }
 
+TEST_F(ThreePhaseUpdateTest, single_full_get_cannot_restart_in_fast_path) {
+    setupDistributor(2, 2, "storage:2 distributor:1");
+    getConfig().set_enable_metadata_only_fetch_phase_for_inconsistent_updates(true);
+    getConfig().set_update_fast_path_restart_enabled(true);
+    auto cb = sendUpdate("0=1/2/3,1=2/3/4"); // Inconsistent replicas.
+    cb->start(_sender, framework::MilliSecTime(0));
+
+    ASSERT_EQ("Get => 0,Get => 1", _sender.getCommands(true));
+    reply_to_metadata_get(*cb, _sender, 0, 1000U);
+    reply_to_metadata_get(*cb, _sender, 1, 2000U);
+    ASSERT_EQ("Get => 1", _sender.getCommands(true, false, 2));
+    replyToGet(*cb, _sender, 2, 2000U);
+    ASSERT_EQ("Put => 1,Put => 0", _sender.getCommands(true, false, 3));
+}
+
 /*
  * We unify checking for changed replica sets and changed bucket ownership by only
  * checking for changed replica sets, thereby avoiding a relatively costly ideal
diff --git a/storage/src/vespa/storage/distributor/operations/external/newest_replica.cpp b/storage/src/vespa/storage/distributor/operations/external/newest_replica.cpp
index d615095ff63..8ca3b9bf411 100644
--- a/storage/src/vespa/storage/distributor/operations/external/newest_replica.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/newest_replica.cpp
@@ -1,3 +1,4 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #include "newest_replica.h"
 #include <ostream>
 
diff --git a/storage/src/vespa/storage/distributor/operations/external/newest_replica.h b/storage/src/vespa/storage/distributor/operations/external/newest_replica.h
index 86d5bee67d2..9eb9c1b8bd0 100644
--- a/storage/src/vespa/storage/distributor/operations/external/newest_replica.h
+++ b/storage/src/vespa/storage/distributor/operations/external/newest_replica.h
@@ -1,3 +1,4 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #pragma once
 
 #include <vespa/document/bucket/bucketid.h>
@@ -7,6 +8,11 @@
 
 namespace storage::distributor {
 
+/*
+ * Tracks the information required to identify the location of the newest replica
+ * for any given document. Newest here means the replica containing the document
+ * version with the highest mutation timestamp.
+ */
 struct NewestReplica {
     api::Timestamp timestamp {0};
     document::BucketId bucket_id;
diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp
index 42ccc3d31f3..dd8f8b553fc 100644
--- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp
@@ -465,7 +465,9 @@ TwoPhaseUpdateOperation::handleSafePathReceivedGet(DistributorMessageSender& sen
         sendReplyWithResult(sender, reply.getResult());
         return;
     }
-    if (may_restart_with_fast_path(reply)) {
+    // Single Get could technically be considered consistent with itself, so make
+    // sure we never treat that as sufficient for restarting in the fast path.
+    if ((_sendState != SendState::SINGLE_GET_SENT) && may_restart_with_fast_path(reply)) {
         restart_with_fast_path_due_to_consistent_get_timestamps(sender);
         return;
     }