Merge pull request #28461 from vespa-engine/vekterli/remove-deprecated-revert-code-from-distributor

Remove deprecated "revert" functionality from distributor code

9 files changed, 7 insertions, 136 deletions

diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp
index ee87fe84df6..8cab6a3003d 100644
--- a/storage/src/tests/distributor/putoperationtest.cpp
+++ b/storage/src/tests/distributor/putoperationtest.cpp
@@ -202,10 +202,7 @@ TEST_F(PutOperationTest, failed_CreateBucket_removes_replica_from_db_and_sends_R
               "node(idx=0,crc=0x1,docs=2/4,bytes=3/5,trusted=true,active=false,ready=false)",
               dumpBucket(operation_context().make_split_bit_constrained_bucket_id(doc->getId())));
 
-    // TODO remove revert concept; does not make sense with Proton (since it's not a multi-version store and
-    //  therefore does not have anything to revert back to) and is config-disabled by default for this provider.
-    ASSERT_EQ("RequestBucketInfoCommand(1 buckets, super bucket BucketId(0x4000000000008f09). ) => 1,"
-              "Revert(BucketId(0x4000000000008f09)) => 0",
+    ASSERT_EQ("RequestBucketInfoCommand(1 buckets, super bucket BucketId(0x4000000000008f09). ) => 1",
               _sender.getCommands(true, true, 4));
 }
 
@@ -430,80 +427,6 @@ TEST_F(PutOperationTest, multiple_copies_early_return_primary_required_not_done)
     ASSERT_EQ(0, _sender.replies().size());
 }
 
-TEST_F(PutOperationTest, do_not_revert_on_failure_after_early_return) {
-    setup_stripe(Redundancy(3),NodeCount(4), "storage:4 distributor:1",
-                     ReturnAfter(2), RequirePrimaryWritten(false));
-
-    sendPut(createPut(createDummyDocument("test", "test")));
-
-    ASSERT_EQ("Create bucket => 3,Create bucket => 2,"
-              "Create bucket => 1,Put => 3,Put => 2,Put => 1",
-              _sender.getCommands(true));
-
-    for (uint32_t i = 0;  i < 3; i++) {
-        sendReply(i); // CreateBucket
-    }
-    for (uint32_t i = 0;  i < 2; i++) {
-        sendReply(3 + i); // Put
-    }
-
-    ASSERT_EQ("PutReply(id:test:testdoctype1::test, BucketId(0x0000000000000000), "
-              "timestamp 100) ReturnCode(NONE)",
-              _sender.getLastReply());
-
-    sendReply(5, api::ReturnCode::INTERNAL_FAILURE);
-    // Should not be any revert commands sent
-    ASSERT_EQ("Create bucket => 3,Create bucket => 2,"
-              "Create bucket => 1,Put => 3,Put => 2,Put => 1",
-              _sender.getCommands(true));
-}
-
-TEST_F(PutOperationTest, revert_successful_copies_when_one_fails) {
-    setup_stripe(3, 4, "storage:4 distributor:1");
-
-    createAndSendSampleDocument(TIMEOUT);
-
-    ASSERT_EQ("Put => 0,Put => 2,Put => 1", _sender.getCommands(true));
-
-    for (uint32_t i = 0;  i < 2; i++) {
-        sendReply(i);
-    }
-
-    sendReply(2, api::ReturnCode::INTERNAL_FAILURE);
-
-    ASSERT_EQ("PutReply(id:test:testdoctype1::, "
-              "BucketId(0x0000000000000000), timestamp 100) "
-              "ReturnCode(INTERNAL_FAILURE)",
-              _sender.getLastReply(true));
-
-    ASSERT_EQ("Revert => 0,Revert => 2", _sender.getCommands(true, false, 3));
-}
-
-TEST_F(PutOperationTest, no_revert_if_revert_disabled) {
-    close();
-    getDirConfig().getConfig("stor-distributormanager")
-                  .set("enable_revert", "false");
-    SetUp();
-    setup_stripe(3, 4, "storage:4 distributor:1");
-
-    createAndSendSampleDocument(TIMEOUT);
-
-    ASSERT_EQ("Put => 0,Put => 2,Put => 1", _sender.getCommands(true));
-
-    for (uint32_t i = 0;  i < 2; i++) {
-        sendReply(i);
-    }
-
-    sendReply(2, api::ReturnCode::INTERNAL_FAILURE);
-
-    ASSERT_EQ("PutReply(id:test:testdoctype1::, "
-              "BucketId(0x0000000000000000), timestamp 100) "
-              "ReturnCode(INTERNAL_FAILURE)",
-              _sender.getLastReply(true));
-
-    ASSERT_EQ("", _sender.getCommands(true, false, 3));
-}
-
 TEST_F(PutOperationTest, do_not_send_CreateBucket_if_already_pending) {
     setup_stripe(2, 2, "storage:2 distributor:1");
 
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp
index 959fdc612cb..ec570820ecd 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.cpp
+++ b/storage/src/vespa/storage/config/distributorconfiguration.cpp
@@ -48,7 +48,6 @@ DistributorConfiguration::DistributorConfiguration(StorageComponent& component)
       _use_weak_internal_read_consistency_for_client_gets(false),
       _enable_metadata_only_fetch_phase_for_inconsistent_updates(false),
       _prioritize_global_bucket_merges(true),
-      _enable_revert(true),
       _implicitly_clear_priority_on_schedule(false),
       _use_unordered_merge_chaining(false),
       _inhibit_default_merges_when_global_merges_pending(false),
@@ -174,7 +173,6 @@ DistributorConfiguration::configure(const vespa::config::content::core::StorDist
     _enable_metadata_only_fetch_phase_for_inconsistent_updates = config.enableMetadataOnlyFetchPhaseForInconsistentUpdates;
     _prioritize_global_bucket_merges = config.prioritizeGlobalBucketMerges;
     _max_activation_inhibited_out_of_sync_groups = config.maxActivationInhibitedOutOfSyncGroups;
-    _enable_revert = config.enableRevert;
     _implicitly_clear_priority_on_schedule = config.implicitlyClearBucketPriorityOnSchedule;
     _use_unordered_merge_chaining = config.useUnorderedMergeChaining;
     _inhibit_default_merges_when_global_merges_pending = config.inhibitDefaultMergesWhenGlobalMergesPending;
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h
index 6056f0a1d5c..08d3e2b055f 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.h
+++ b/storage/src/vespa/storage/config/distributorconfiguration.h
@@ -263,9 +263,6 @@ public:
         return _max_activation_inhibited_out_of_sync_groups;
     }
 
-    bool enable_revert() const noexcept {
-        return _enable_revert;
-    }
     [[nodiscard]] bool implicitly_clear_priority_on_schedule() const noexcept {
         return _implicitly_clear_priority_on_schedule;
     }
@@ -354,7 +351,6 @@ private:
     bool _use_weak_internal_read_consistency_for_client_gets;
     bool _enable_metadata_only_fetch_phase_for_inconsistent_updates;
     bool _prioritize_global_bucket_merges;
-    bool _enable_revert;
     bool _implicitly_clear_priority_on_schedule;
     bool _use_unordered_merge_chaining;
     bool _inhibit_default_merges_when_global_merges_pending;
diff --git a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
index 5735e47ec87..e4defda2bb0 100644
--- a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp
@@ -28,8 +28,7 @@ PutOperation::PutOperation(const DistributorNodeContext& node_ctx,
                            PersistenceOperationMetricSet& condition_probe_metrics,
                            SequencingHandle sequencing_handle)
     : SequencedOperation(std::move(sequencing_handle)),
-      _tracker(metric, std::make_shared<api::PutReply>(*msg), node_ctx,
-               op_ctx, _cancel_scope, msg->getTimestamp()),
+      _tracker(metric, std::make_shared<api::PutReply>(*msg), node_ctx, op_ctx, _cancel_scope),
       _msg(std::move(msg)),
       _doc_id_bucket_id(document::BucketIdFactory{}.getBucketId(_msg->getDocumentId())),
       _node_ctx(node_ctx),
diff --git a/storage/src/vespa/storage/distributor/operations/external/removelocationoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/removelocationoperation.cpp
index ab3855b2687..07112add6e3 100644
--- a/storage/src/vespa/storage/distributor/operations/external/removelocationoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/removelocationoperation.cpp
@@ -23,7 +23,7 @@ RemoveLocationOperation::RemoveLocationOperation(
         std::shared_ptr<api::RemoveLocationCommand> msg,
         PersistenceOperationMetricSet& metric)
     : Operation(),
-      _tracker(metric, std::make_shared<api::RemoveLocationReply>(*msg), node_ctx, op_ctx, _cancel_scope, 0),
+      _tracker(metric, std::make_shared<api::RemoveLocationReply>(*msg), node_ctx, op_ctx, _cancel_scope),
       _msg(std::move(msg)),
       _node_ctx(node_ctx),
       _parser(parser),
diff --git a/storage/src/vespa/storage/distributor/operations/external/removeoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/removeoperation.cpp
index c7cfea017ac..7b38f8ca21e 100644
--- a/storage/src/vespa/storage/distributor/operations/external/removeoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/removeoperation.cpp
@@ -19,8 +19,7 @@ RemoveOperation::RemoveOperation(const DistributorNodeContext& node_ctx,
                                  PersistenceOperationMetricSet& condition_probe_metrics,
                                  SequencingHandle sequencingHandle)
     : SequencedOperation(std::move(sequencingHandle)),
-      _tracker(metric, std::make_shared<api::RemoveReply>(*msg), node_ctx,
-               op_ctx, _cancel_scope, msg->getTimestamp()),
+      _tracker(metric, std::make_shared<api::RemoveReply>(*msg), node_ctx, op_ctx, _cancel_scope),
       _msg(std::move(msg)),
       _doc_id_bucket_id(document::BucketIdFactory{}.getBucketId(_msg->getDocumentId())),
       _node_ctx(node_ctx),
diff --git a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp
index 90f3f8c9c43..6c80a192ab3 100644
--- a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp
@@ -25,8 +25,7 @@ UpdateOperation::UpdateOperation(const DistributorNodeContext& node_ctx,
                                  std::vector<BucketDatabase::Entry> entries,
                                  UpdateMetricSet& metric)
     : Operation(),
-      _tracker(metric, std::make_shared<api::UpdateReply>(*msg), node_ctx,
-               op_ctx, _cancel_scope, msg->getTimestamp()),
+      _tracker(metric, std::make_shared<api::UpdateReply>(*msg), node_ctx, op_ctx, _cancel_scope),
       _msg(msg),
       _entries(std::move(entries)),
       _new_timestamp(_msg->getTimestamp()),
diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
index 43e3f6831d4..a0c4d6786f6 100644
--- a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
+++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
@@ -18,15 +18,13 @@ PersistenceMessageTracker::PersistenceMessageTracker(
         std::shared_ptr<api::BucketInfoReply> reply,
         const DistributorNodeContext& node_ctx,
         DistributorStripeOperationContext& op_ctx,
-        CancelScope& cancel_scope,
-        api::Timestamp revertTimestamp)
+        CancelScope& cancel_scope)
     : MessageTracker(node_ctx),
       _remapBucketInfo(),
       _bucketInfo(),
       _metric(metric),
       _reply(std::move(reply)),
       _op_ctx(op_ctx),
-      _revertTimestamp(revertTimestamp),
       _trace(_reply->getTrace().getLevel()),
       _requestTimer(node_ctx.clock()),
       _cancel_scope(cancel_scope),
@@ -109,26 +107,6 @@ PersistenceMessageTracker::receiveReply(MessageSender& sender, api::BucketInfoRe
 }
 
 void
-PersistenceMessageTracker::revert(MessageSender& sender, const std::vector<BucketNodePair>& revertNodes)
-{
-    if (_revertTimestamp != 0) {
-        // Since we're reverting, all received bucket info is voided.
-        _bucketInfo.clear();
-
-        std::vector<api::Timestamp> reverts;
-        reverts.push_back(_revertTimestamp);
-
-        for (const auto & revertNode : revertNodes) {
-            auto toRevert = std::make_shared<api::RevertCommand>(revertNode.first, reverts);
-            toRevert->setPriority(_priority);
-            queueCommand(std::move(toRevert), revertNode.second);
-        }
-
-        flushQueue(sender);
-    }
-}
-
-void
 PersistenceMessageTracker::queueMessageBatch(std::vector<MessageTracker::ToSend> messages) {
     _messageBatches.emplace_back();
     auto & batch = _messageBatches.back();
@@ -213,13 +191,6 @@ PersistenceMessageTracker::logSuccessfulReply(uint16_t node, const api::BucketIn
     }
 }
 
-bool
-PersistenceMessageTracker::shouldRevert() const
-{
-    return _op_ctx.distributor_config().enable_revert()
-            &&  !_revertNodes.empty() && !_success && _reply;
-}
-
 bool PersistenceMessageTracker::has_majority_successful_replies() const noexcept {
     // FIXME this has questionable interaction with early client ACK since we only count
     // the number of observed replies rather than the number of total requests sent.
@@ -296,7 +267,6 @@ PersistenceMessageTracker::handlePersistenceReply(api::BucketInfoReply& reply, u
     }
     if (reply.getResult().success()) {
         logSuccessfulReply(node, reply);
-        _revertNodes.emplace_back(reply.getBucket(), node);
         ++_n_successful_persistence_replies;
     } else if (!hasSentReply()) {
         updateFailureResult(reply);
@@ -324,16 +294,11 @@ PersistenceMessageTracker::updateFromReply(MessageSender& sender, api::BucketInf
     }
 
     if (finished()) {
-        bool doRevert(shouldRevert());
-
         updateDB();
 
         if (!hasSentReply()) {
             sendReply(sender);
         }
-        if (doRevert) {
-            revert(sender, _revertNodes);
-        }
     } else if (canSendReplyEarly()) {
         LOG(debug, "Sending reply early because initial redundancy has been reached");
         sendReply(sender);
diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.h b/storage/src/vespa/storage/distributor/persistencemessagetracker.h
index 06ad8dc95b3..00e97b12a94 100644
--- a/storage/src/vespa/storage/distributor/persistencemessagetracker.h
+++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.h
@@ -19,8 +19,7 @@ public:
                               std::shared_ptr<api::BucketInfoReply> reply,
                               const DistributorNodeContext& node_ctx,
                               DistributorStripeOperationContext& op_ctx,
-                              CancelScope& cancel_scope,
-                              api::Timestamp revertTimestamp = 0);
+                              CancelScope& cancel_scope);
     ~PersistenceMessageTracker();
 
     void updateDB();
@@ -35,10 +34,6 @@ public:
     void updateFromReply(MessageSender& sender, api::BucketInfoReply& reply, uint16_t node);
     std::shared_ptr<api::BucketInfoReply>& getReply() { return _reply; }
 
-    using BucketNodePair = std::pair<document::Bucket, uint16_t>;
-
-    void revert(MessageSender& sender, const std::vector<BucketNodePair>& revertNodes);
-
     /**
        Sends a set of messages that are permissible for early return.
        If early return is enabled, each message batch must be "finished", that is,
@@ -59,8 +54,6 @@ private:
     PersistenceOperationMetricSet&        _metric;
     std::shared_ptr<api::BucketInfoReply> _reply;
     DistributorStripeOperationContext&    _op_ctx;
-    api::Timestamp                        _revertTimestamp;
-    std::vector<BucketNodePair>           _revertNodes;
     mbus::Trace                           _trace;
     framework::MilliSecTimer              _requestTimer;
     CancelScope&                          _cancel_scope;
@@ -86,7 +79,6 @@ private:
     void addBucketInfoFromReply(uint16_t node, const api::BucketInfoReply& reply);
     void logSuccessfulReply(uint16_t node, const api::BucketInfoReply& reply) const;
     [[nodiscard]] bool hasSentReply() const noexcept { return !_reply; }
-    [[nodiscard]] bool shouldRevert() const;
     [[nodiscard]] bool has_majority_successful_replies() const noexcept;
     [[nodiscard]] bool has_minority_test_and_set_failure() const noexcept;
     void sendReply(MessageSender& sender);