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".

9 files changed, 69 insertions, 12 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()