Add wrapper for <doc id, timestamp> tuple and update APIs to use this

Feels more intuitive to have a tuple that implies "document foo at timestamp bar"
rather than the current inverse of "timestamp bar with document foo".

18 files changed, 92 insertions, 35 deletions

diff --git a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
index bf1c828e2e6..e03403f601d 100644
--- a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
+++ b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
@@ -765,11 +765,11 @@ TEST_F(ConformanceTest, testRemoveMulti)
         docs.push_back(testDocMan.createRandomDocumentAtLocation(0x01, i));
     }
 
-    std::vector<PersistenceProvider::TimeStampAndDocumentId> ids;
+    std::vector<spi::IdAndTimestamp> ids;
     for (size_t i(0); i < docs.size(); i++) {
         spi->put(bucket1, Timestamp(i), docs[i]);
         if (i & 0x1) {
-            ids.emplace_back(Timestamp(i), docs[i]->getId());
+            ids.emplace_back(docs[i]->getId(), Timestamp(i));
         }
     }
 
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
index 81bfdf7f9a3..6eabadc2f86 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
@@ -476,16 +476,16 @@ DummyPersistence::updateAsync(const Bucket& bucket, Timestamp ts, DocumentUpdate
 }
 
 void
-DummyPersistence::removeAsync(const Bucket& b, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP onComplete)
+DummyPersistence::removeAsync(const Bucket& b, std::vector<spi::IdAndTimestamp> ids, OperationComplete::UP onComplete)
 {
     DUMMYPERSISTENCE_VERIFY_INITIALIZED;
     assert(b.getBucketSpace() == FixedBucketSpaces::default_space());
     BucketContentGuard::UP bc(acquireBucketWithLock(b));
 
     uint32_t numRemoves(0);
-    for (const TimeStampAndDocumentId & stampedId : ids) {
-        const DocumentId & id = stampedId.second;
-        Timestamp t = stampedId.first;
+    for (const spi::IdAndTimestamp & stampedId : ids) {
+        const DocumentId & id = stampedId.id;
+        Timestamp t = stampedId.timestamp;
         LOG(debug, "remove(%s, %" PRIu64 ", %s)", b.toString().c_str(), uint64_t(t), id.toString().c_str());
 
         while (!bc) {
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
index e015185e5b0..56602b3ab00 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
@@ -160,7 +160,7 @@ public:
     BucketInfoResult getBucketInfo(const Bucket&) const override;
     GetResult get(const Bucket&, const document::FieldSet&, const DocumentId&, Context&) const override;
     void putAsync(const Bucket&, Timestamp, DocumentSP, OperationComplete::UP) override;
-    void removeAsync(const Bucket& b, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP) override;
+    void removeAsync(const Bucket& b, std::vector<spi::IdAndTimestamp> ids, OperationComplete::UP) override;
     void updateAsync(const Bucket&, Timestamp, DocumentUpdateSP, OperationComplete::UP) override;
 
     CreateIteratorResult
diff --git a/persistence/src/vespa/persistence/spi/CMakeLists.txt b/persistence/src/vespa/persistence/spi/CMakeLists.txt
index e4bae1c7551..bc94020ae95 100644
--- a/persistence/src/vespa/persistence/spi/CMakeLists.txt
+++ b/persistence/src/vespa/persistence/spi/CMakeLists.txt
@@ -10,6 +10,7 @@ vespa_add_library(persistence_spi OBJECT
     context.cpp
     docentry.cpp
     exceptions.cpp
+    id_and_timestamp.cpp
     persistenceprovider.cpp
     read_consistency.cpp
     resource_usage.cpp
diff --git a/persistence/src/vespa/persistence/spi/abstractpersistenceprovider.cpp b/persistence/src/vespa/persistence/spi/abstractpersistenceprovider.cpp
index f301a9c5428..04d06235f59 100644
--- a/persistence/src/vespa/persistence/spi/abstractpersistenceprovider.cpp
+++ b/persistence/src/vespa/persistence/spi/abstractpersistenceprovider.cpp
@@ -10,8 +10,8 @@ void
 AbstractPersistenceProvider::removeIfFoundAsync(const Bucket& b, Timestamp timestamp,
                                                 const DocumentId& id, OperationComplete::UP onComplete)
 {
-    std::vector<TimeStampAndDocumentId> ids;
-    ids.emplace_back(timestamp, id);
+    std::vector<IdAndTimestamp> ids;
+    ids.emplace_back(id, timestamp);
     removeAsync(b, std::move(ids), std::move(onComplete));
 }
 
diff --git a/persistence/src/vespa/persistence/spi/id_and_timestamp.cpp b/persistence/src/vespa/persistence/spi/id_and_timestamp.cpp
new file mode 100644
index 00000000000..fba45990744
--- /dev/null
+++ b/persistence/src/vespa/persistence/spi/id_and_timestamp.cpp
@@ -0,0 +1,17 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+#include "id_and_timestamp.h"
+
+namespace storage::spi {
+
+IdAndTimestamp::IdAndTimestamp() : id(), timestamp(0) {}
+IdAndTimestamp::IdAndTimestamp(document::DocumentId id_, Timestamp timestamp_) noexcept
+    : id(std::move(id_)),
+      timestamp(timestamp_)
+{}
+
+IdAndTimestamp::IdAndTimestamp(const IdAndTimestamp&) = default;
+IdAndTimestamp& IdAndTimestamp::operator=(const IdAndTimestamp&) = default;
+IdAndTimestamp::IdAndTimestamp(IdAndTimestamp&&) noexcept = default;
+IdAndTimestamp& IdAndTimestamp::operator=(IdAndTimestamp&&) noexcept = default;
+
+}
diff --git a/persistence/src/vespa/persistence/spi/id_and_timestamp.h b/persistence/src/vespa/persistence/spi/id_and_timestamp.h
new file mode 100644
index 00000000000..d8cdba3d063
--- /dev/null
+++ b/persistence/src/vespa/persistence/spi/id_and_timestamp.h
@@ -0,0 +1,38 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+#pragma once
+
+#include "types.h"
+#include <vespa/document/base/documentid.h>
+
+namespace storage::spi {
+
+/**
+ * Convenience wrapper for referencing a document ID at a particular timestamp.
+ *
+ * Prefer this instead of a std::pair due to named fields and a pre-provided hash function.
+ */
+struct IdAndTimestamp {
+    document::DocumentId id;
+    Timestamp timestamp;
+
+    IdAndTimestamp();
+    IdAndTimestamp(document::DocumentId id_, Timestamp timestamp_) noexcept;
+
+    IdAndTimestamp(const IdAndTimestamp&);
+    IdAndTimestamp& operator=(const IdAndTimestamp&);
+    IdAndTimestamp(IdAndTimestamp&&) noexcept;
+    IdAndTimestamp& operator=(IdAndTimestamp&&) noexcept;
+
+    bool operator==(const IdAndTimestamp& rhs) const noexcept {
+        return ((id == rhs.id) && (timestamp == rhs.timestamp));
+    }
+
+    struct hash {
+        size_t operator()(const IdAndTimestamp& id_ts) const noexcept {
+            const size_t h = document::GlobalId::hash()(id_ts.id.getGlobalId());
+            return h ^ (id_ts.timestamp + 0x9e3779b9U + (h << 6U) + (h >> 2U)); // Basically boost::hash_combine
+        }
+    };
+};
+
+}
diff --git a/persistence/src/vespa/persistence/spi/persistenceprovider.cpp b/persistence/src/vespa/persistence/spi/persistenceprovider.cpp
index 03cefb8df89..911b3753b1f 100644
--- a/persistence/src/vespa/persistence/spi/persistenceprovider.cpp
+++ b/persistence/src/vespa/persistence/spi/persistenceprovider.cpp
@@ -44,8 +44,8 @@ RemoveResult
 PersistenceProvider::remove(const Bucket& bucket, Timestamp timestamp, const DocumentId & docId) {
     auto catcher = std::make_unique<CatchResult>();
     auto future = catcher->future_result();
-    std::vector<TimeStampAndDocumentId> ids;
-    ids.emplace_back(timestamp, docId);
+    std::vector<IdAndTimestamp> ids;
+    ids.emplace_back(docId, timestamp);
     removeAsync(bucket, std::move(ids), std::move(catcher));
     return dynamic_cast<const RemoveResult &>(*future.get());
 }
diff --git a/persistence/src/vespa/persistence/spi/persistenceprovider.h b/persistence/src/vespa/persistence/spi/persistenceprovider.h
index a90d39e8334..d3e1465e528 100644
--- a/persistence/src/vespa/persistence/spi/persistenceprovider.h
+++ b/persistence/src/vespa/persistence/spi/persistenceprovider.h
@@ -4,6 +4,7 @@
 #include "bucket.h"
 #include "bucketinfo.h"
 #include "context.h"
+#include "id_and_timestamp.h"
 #include "result.h"
 #include "selection.h"
 #include "clusterstate.h"
@@ -170,7 +171,7 @@ struct PersistenceProvider
      * @param timestamp The timestamp for the new bucket entry.
      * @param id The ID to remove
      */
-    virtual void removeAsync(const Bucket&, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP) = 0;
+    virtual void removeAsync(const Bucket&, std::vector<IdAndTimestamp> ids, OperationComplete::UP) = 0;
 
     /**
      * @see remove()
diff --git a/searchcore/src/vespa/searchcore/bmcluster/spi_bm_feed_handler.cpp b/searchcore/src/vespa/searchcore/bmcluster/spi_bm_feed_handler.cpp
index 69013e8d7c5..dcdba3b0715 100644
--- a/searchcore/src/vespa/searchcore/bmcluster/spi_bm_feed_handler.cpp
+++ b/searchcore/src/vespa/searchcore/bmcluster/spi_bm_feed_handler.cpp
@@ -134,8 +134,8 @@ SpiBmFeedHandler::remove(const document::Bucket& bucket, const DocumentId& docum
     auto provider = get_provider(bucket);
     if (provider) {
         Bucket spi_bucket(bucket);
-        std::vector<storage::spi::PersistenceProvider::TimeStampAndDocumentId> ids;
-        ids.emplace_back(Timestamp(timestamp), document_id);
+        std::vector<storage::spi::IdAndTimestamp> ids;
+        ids.emplace_back(document_id, Timestamp(timestamp));
         provider->removeAsync(spi_bucket, std::move(ids), std::make_unique<MyOperationComplete>(provider, _errors, spi_bucket, tracker));
     } else {
         ++_errors;
diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
index 5a0bcb1cd41..81a7244ba1d 100644
--- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
+++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
@@ -370,21 +370,21 @@ PersistenceEngine::putAsync(const Bucket &bucket, Timestamp ts, storage::spi::Do
 }
 
 void
-PersistenceEngine::removeAsync(const Bucket& b, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP onComplete)
+PersistenceEngine::removeAsync(const Bucket& b, std::vector<storage::spi::IdAndTimestamp> ids, OperationComplete::UP onComplete)
 {
     if (ids.size() == 1) {
-        removeAsyncSingle(b, ids[0].first, ids[0].second, std::move(onComplete));
+        removeAsyncSingle(b, ids[0].timestamp, ids[0].id, std::move(onComplete));
     } else {
         removeAsyncMulti(b, std::move(ids), std::move(onComplete));
     }
 }
 
 void
-PersistenceEngine::removeAsyncMulti(const Bucket& b, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP onComplete) {
+PersistenceEngine::removeAsyncMulti(const Bucket& b, std::vector<storage::spi::IdAndTimestamp> ids, OperationComplete::UP onComplete) {
     ReadGuard rguard(_rwMutex);
     //TODO Group per document type/handler and handle in one go.
-    for (const TimeStampAndDocumentId & stampedId : ids) {
-        const document::DocumentId & id = stampedId.second;
+    for (const auto & stampedId : ids) {
+        const document::DocumentId & id = stampedId.id;
         if (!id.hasDocType()) {
             return onComplete->onComplete(
                     std::make_unique<RemoveResult>(Result::ErrorType::PERMANENT_ERROR,
@@ -399,11 +399,11 @@ PersistenceEngine::removeAsyncMulti(const Bucket& b, std::vector<TimeStampAndDoc
         }
     }
     auto transportContext = std::make_shared<AsyncRemoveTransportContext>(ids.size(), std::move(onComplete));
-    for (const TimeStampAndDocumentId & stampedId : ids) {
-        const document::DocumentId & id = stampedId.second;
+    for (const auto & stampedId : ids) {
+        const document::DocumentId & id = stampedId.id;
         DocTypeName docType(id.getDocType());
         IPersistenceHandler *handler = getHandler(rguard, b.getBucketSpace(), docType);
-        handler->handleRemove(feedtoken::make(transportContext), b, stampedId.first, id);
+        handler->handleRemove(feedtoken::make(transportContext), b, stampedId.timestamp, id);
     }
 }
 
diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
index a8886e19def..c16cc6e6a83 100644
--- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
+++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
@@ -89,7 +89,7 @@ private:
     ClusterState::SP savedClusterState(BucketSpace bucketSpace) const;
     std::shared_ptr<BucketExecutor> get_bucket_executor() noexcept { return _bucket_executor.lock(); }
     void removeAsyncSingle(const Bucket&, Timestamp, const document::DocumentId &id, OperationComplete::UP);
-    void removeAsyncMulti(const Bucket&, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP);
+    void removeAsyncMulti(const Bucket&, std::vector<storage::spi::IdAndTimestamp> ids, OperationComplete::UP);
 public:
     typedef std::unique_ptr<PersistenceEngine> UP;
 
@@ -107,7 +107,7 @@ public:
     void setActiveStateAsync(const Bucket&, BucketInfo::ActiveState, OperationComplete::UP) override;
     BucketInfoResult getBucketInfo(const Bucket&) const override;
     void putAsync(const Bucket &, Timestamp, storage::spi::DocumentSP, OperationComplete::UP) override;
-    void removeAsync(const Bucket&, std::vector<TimeStampAndDocumentId> ids, OperationComplete::UP) override;
+    void removeAsync(const Bucket&, std::vector<storage::spi::IdAndTimestamp> ids, OperationComplete::UP) override;
     void updateAsync(const Bucket&, Timestamp, storage::spi::DocumentUpdateSP, OperationComplete::UP) override;
     GetResult get(const Bucket&, const document::FieldSet&, const document::DocumentId&, Context&) const override;
     CreateIteratorResult
diff --git a/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp b/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp
index 7e0b96b1d82..fcb56c4a553 100644
--- a/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp
+++ b/storage/src/tests/persistence/common/persistenceproviderwrapper.cpp
@@ -106,11 +106,11 @@ PersistenceProviderWrapper::putAsync(const spi::Bucket& bucket, spi::Timestamp t
 }
 
 void
-PersistenceProviderWrapper::removeAsync(const spi::Bucket& bucket,  std::vector<TimeStampAndDocumentId> ids,
+PersistenceProviderWrapper::removeAsync(const spi::Bucket& bucket,  std::vector<spi::IdAndTimestamp> ids,
                                         spi::OperationComplete::UP onComplete)
 {
-    for (const TimeStampAndDocumentId & stampedId : ids) {
-        LOG_SPI("remove(" << bucket << ", " << stampedId.first << ", " << stampedId.second << ")");
+    for (const auto & stampedId : ids) {
+        LOG_SPI("remove(" << bucket << ", " << stampedId.timestamp << ", " << stampedId.id << ")");
     }
     CHECK_ERROR_ASYNC(spi::RemoveResult, FAIL_REMOVE, onComplete);
     _spi.removeAsync(bucket, std::move(ids), std::move(onComplete));
diff --git a/storage/src/tests/persistence/common/persistenceproviderwrapper.h b/storage/src/tests/persistence/common/persistenceproviderwrapper.h
index 3c93bc91d85..ec7ca70d7c7 100644
--- a/storage/src/tests/persistence/common/persistenceproviderwrapper.h
+++ b/storage/src/tests/persistence/common/persistenceproviderwrapper.h
@@ -106,7 +106,7 @@ public:
     spi::BucketIdListResult listBuckets(BucketSpace bucketSpace) const override;
     spi::BucketInfoResult getBucketInfo(const spi::Bucket&) const override;
     void putAsync(const spi::Bucket&, spi::Timestamp, spi::DocumentSP, spi::OperationComplete::UP) override;
-    void removeAsync(const spi::Bucket&, std::vector<TimeStampAndDocumentId> ids, spi::OperationComplete::UP) override;
+    void removeAsync(const spi::Bucket&, std::vector<spi::IdAndTimestamp> ids, spi::OperationComplete::UP) override;
     void removeIfFoundAsync(const spi::Bucket&, spi::Timestamp, const spi::DocumentId&, spi::OperationComplete::UP) override;
     void updateAsync(const spi::Bucket&, spi::Timestamp, spi::DocumentUpdateSP, spi::OperationComplete::UP) override;
     spi::GetResult get(const spi::Bucket&, const document::FieldSet&, const spi::DocumentId&, spi::Context&) const override;
diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp
index f5d29fb32a7..d5bf733a30c 100644
--- a/storage/src/vespa/storage/persistence/asynchandler.cpp
+++ b/storage/src/vespa/storage/persistence/asynchandler.cpp
@@ -114,13 +114,13 @@ bucketStatesAreSemanticallyEqual(const api::BucketInfo& a, const api::BucketInfo
 
 class UnrevertableRemoveEntryProcessor : public BucketProcessor::EntryProcessor {
 public:
-    using DocumentIdsAndTimeStamps = std::vector<std::pair<spi::Timestamp, spi::DocumentId>>;
+    using DocumentIdsAndTimeStamps = std::vector<spi::IdAndTimestamp>;
     UnrevertableRemoveEntryProcessor(DocumentIdsAndTimeStamps & to_remove)
         : _to_remove(to_remove)
     {}
 
     void process(spi::DocEntry& entry) override {
-        _to_remove.emplace_back(entry.getTimestamp(), *entry.getDocumentId());
+        _to_remove.emplace_back(*entry.getDocumentId(), entry.getTimestamp());
     }
 private:
     DocumentIdsAndTimeStamps & _to_remove;
diff --git a/storage/src/vespa/storage/persistence/mergehandler.cpp b/storage/src/vespa/storage/persistence/mergehandler.cpp
index 012d5c2619d..ae68a694c90 100644
--- a/storage/src/vespa/storage/persistence/mergehandler.cpp
+++ b/storage/src/vespa/storage/persistence/mergehandler.cpp
@@ -522,9 +522,9 @@ MergeHandler::applyDiffEntry(std::shared_ptr<ApplyBucketDiffState> async_results
                                                                        _clock, _env._metrics.merge_handler_metrics.put_latency);
         _spi.putAsync(bucket, timestamp, std::move(doc), std::move(complete));
     } else {
-        std::vector<spi::PersistenceProvider::TimeStampAndDocumentId> ids;
-        ids.emplace_back(timestamp, e._docName);
-        auto complete = std::make_unique<ApplyBucketDiffEntryComplete>(std::move(async_results), ids[0].second,
+        std::vector<spi::IdAndTimestamp> ids;
+        ids.emplace_back(document::DocumentId(e._docName), timestamp);
+        auto complete = std::make_unique<ApplyBucketDiffEntryComplete>(std::move(async_results), ids[0].id,
                                                                        std::move(throttle_token), "remove",
                                                                        _clock, _env._metrics.merge_handler_metrics.remove_latency);
         _spi.removeAsync(bucket, std::move(ids), std::move(complete));
diff --git a/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp b/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp
index 1be9679c641..9e55c0f9088 100644
--- a/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp
+++ b/storage/src/vespa/storage/persistence/provider_error_wrapper.cpp
@@ -152,7 +152,7 @@ ProviderErrorWrapper::putAsync(const spi::Bucket &bucket, spi::Timestamp ts, spi
 }
 
 void
-ProviderErrorWrapper::removeAsync(const spi::Bucket &bucket, std::vector<TimeStampAndDocumentId> ids,
+ProviderErrorWrapper::removeAsync(const spi::Bucket &bucket, std::vector<spi::IdAndTimestamp> ids,
                                   spi::OperationComplete::UP onComplete)
 {
     onComplete->addResultHandler(this);
diff --git a/storage/src/vespa/storage/persistence/provider_error_wrapper.h b/storage/src/vespa/storage/persistence/provider_error_wrapper.h
index 7bd406a8758..82447fe4549 100644
--- a/storage/src/vespa/storage/persistence/provider_error_wrapper.h
+++ b/storage/src/vespa/storage/persistence/provider_error_wrapper.h
@@ -58,7 +58,7 @@ public:
     void register_error_listener(std::shared_ptr<ProviderErrorListener> listener);
 
     void putAsync(const spi::Bucket &, spi::Timestamp, spi::DocumentSP, spi::OperationComplete::UP) override;
-    void removeAsync(const spi::Bucket&, std::vector<TimeStampAndDocumentId>, spi::OperationComplete::UP) override;
+    void removeAsync(const spi::Bucket&, std::vector<spi::IdAndTimestamp>, spi::OperationComplete::UP) override;
     void removeIfFoundAsync(const spi::Bucket&, spi::Timestamp, const document::DocumentId&, spi::OperationComplete::UP) override;
     void updateAsync(const spi::Bucket &, spi::Timestamp, spi::DocumentUpdateSP, spi::OperationComplete::UP) override;
     void setActiveStateAsync(const spi::Bucket& b, spi::BucketInfo::ActiveState newState, spi::OperationComplete::UP onComplete) override;