From 2c8db3de8a721af856fee825af5a48d10313ff1e Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Mon, 25 May 2020 11:56:09 +0000 Subject: Propagate provider tombstone metadata to internal SPI GetResult --- .../conformancetest/conformancetest.cpp | 24 +++++++++++----- .../persistence/dummyimpl/dummypersistence.cpp | 5 +++- persistence/src/vespa/persistence/spi/result.cpp | 13 +++++++-- persistence/src/vespa/persistence/spi/result.h | 33 ++++++++++++++++++---- .../persistenceengine/persistenceengine_test.cpp | 1 + .../proton/persistenceengine/persistenceengine.cpp | 2 +- .../storage/persistence/persistencethread.cpp | 3 +- 7 files changed, 63 insertions(+), 18 deletions(-) diff --git a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp index b45a4131e7c..036a2b87692 100644 --- a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp +++ b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp @@ -625,6 +625,7 @@ TEST_F(ConformanceTest, testPutNewDocumentVersion) EXPECT_EQ(Result::ErrorType::NONE, gr.getErrorCode()); EXPECT_EQ(Timestamp(4), gr.getTimestamp()); + EXPECT_FALSE(gr.is_tombstone()); if (!((*doc2)==gr.getDocument())) { std::cerr << "Document returned is not the expected one: \n" @@ -676,6 +677,7 @@ TEST_F(ConformanceTest, testPutOlderDocumentVersion) EXPECT_EQ(Result::ErrorType::NONE, gr.getErrorCode()); EXPECT_EQ(Timestamp(5), gr.getTimestamp()); EXPECT_EQ(*doc1, gr.getDocument()); + EXPECT_FALSE(gr.is_tombstone()); } TEST_F(ConformanceTest, testPutDuplicate) @@ -798,8 +800,9 @@ TEST_F(ConformanceTest, testRemove) context); EXPECT_EQ(Result::ErrorType::NONE, getResult.getErrorCode()); - EXPECT_EQ(Timestamp(0), getResult.getTimestamp()); - EXPECT_TRUE(!getResult.hasDocument()); + EXPECT_EQ(Timestamp(9), getResult.getTimestamp()); + EXPECT_TRUE(getResult.is_tombstone()); + EXPECT_FALSE(getResult.hasDocument()); } TEST_F(ConformanceTest, testRemoveMerge) @@ -939,6 +942,7 @@ TEST_F(ConformanceTest, testUpdate) EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); EXPECT_EQ(Timestamp(4), result.getTimestamp()); + EXPECT_FALSE(result.is_tombstone()); EXPECT_EQ(document::IntFieldValue(42), static_cast( *result.getDocument().getValue("headerval"))); @@ -953,8 +957,9 @@ TEST_F(ConformanceTest, testUpdate) context); EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); - EXPECT_EQ(Timestamp(0), result.getTimestamp()); - EXPECT_TRUE(!result.hasDocument()); + EXPECT_EQ(Timestamp(5), result.getTimestamp()); + EXPECT_FALSE(result.hasDocument()); + EXPECT_TRUE(result.is_tombstone()); } { @@ -968,8 +973,9 @@ TEST_F(ConformanceTest, testUpdate) { GetResult result = spi->get(bucket, document::AllFields(), doc1->getId(), context); EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); - EXPECT_EQ(Timestamp(0), result.getTimestamp()); - EXPECT_TRUE(!result.hasDocument()); + EXPECT_EQ(Timestamp(5), result.getTimestamp()); + EXPECT_FALSE(result.hasDocument()); + EXPECT_TRUE(result.is_tombstone()); } update->setCreateIfNonExistent(true); @@ -985,6 +991,7 @@ TEST_F(ConformanceTest, testUpdate) GetResult result = spi->get(bucket, document::AllFields(), doc1->getId(), context); EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); EXPECT_EQ(Timestamp(7), result.getTimestamp()); + EXPECT_FALSE(result.is_tombstone()); EXPECT_EQ(document::IntFieldValue(42), reinterpret_cast( *result.getDocument().getValue("headerval"))); @@ -1008,6 +1015,7 @@ TEST_F(ConformanceTest, testGet) EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); EXPECT_EQ(Timestamp(0), result.getTimestamp()); + EXPECT_FALSE(result.is_tombstone()); } spi->put(bucket, Timestamp(3), doc1, context); @@ -1017,6 +1025,7 @@ TEST_F(ConformanceTest, testGet) doc1->getId(), context); EXPECT_EQ(*doc1, result.getDocument()); EXPECT_EQ(Timestamp(3), result.getTimestamp()); + EXPECT_FALSE(result.is_tombstone()); } spi->remove(bucket, Timestamp(4), doc1->getId(), context); @@ -1026,7 +1035,8 @@ TEST_F(ConformanceTest, testGet) doc1->getId(), context); EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode()); - EXPECT_EQ(Timestamp(0), result.getTimestamp()); + EXPECT_EQ(Timestamp(4), result.getTimestamp()); + EXPECT_TRUE(result.is_tombstone()); } } diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp index 5720a0ba662..92fea5ff6e4 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp @@ -533,7 +533,10 @@ DummyPersistence::get(const Bucket& b, const document::FieldSet& fieldSet, const if (!bc.get()) { } else { DocEntry::SP entry((*bc)->getEntry(did)); - if (entry.get() == 0 || entry->isRemove()) { + if (!entry) { + return GetResult(); + } else if (entry->isRemove()) { + return GetResult::make_for_tombstone(entry->getTimestamp()); } else { Document::UP doc(entry->getDocument()->clone()); if (fieldSet.getType() != document::FieldSet::ALL) { diff --git a/persistence/src/vespa/persistence/spi/result.cpp b/persistence/src/vespa/persistence/spi/result.cpp index f5131f08b1f..8de6245a8d2 100644 --- a/persistence/src/vespa/persistence/spi/result.cpp +++ b/persistence/src/vespa/persistence/spi/result.cpp @@ -30,8 +30,17 @@ std::ostream & operator << (std::ostream & os, const Result::ErrorType &errorCod GetResult::GetResult(Document::UP doc, Timestamp timestamp) : Result(), _timestamp(timestamp), - _doc(std::move(doc)) -{ } + _doc(std::move(doc)), + _is_tombstone(false) +{ +} + +GetResult::GetResult(Timestamp removed_at_ts) + : _timestamp(removed_at_ts), + _doc(), + _is_tombstone(true) +{ +} GetResult::~GetResult() = default; BucketIdListResult::~BucketIdListResult() = default; diff --git a/persistence/src/vespa/persistence/spi/result.h b/persistence/src/vespa/persistence/spi/result.h index fe8e74706bb..714d71e37ee 100644 --- a/persistence/src/vespa/persistence/spi/result.h +++ b/persistence/src/vespa/persistence/spi/result.h @@ -156,13 +156,20 @@ public: */ GetResult(ErrorType error, const vespalib::string& errorMessage) : Result(error, errorMessage), - _timestamp(0) { } + _timestamp(0), + _is_tombstone(false) + { + } /** * Constructor to use when we didn't find the document in question. */ GetResult() - : _timestamp(0) { } + : _timestamp(0), + _doc(), + _is_tombstone(false) + { + } /** * Constructor to use when we found the document asked for. @@ -172,12 +179,22 @@ public: */ GetResult(DocumentUP doc, Timestamp timestamp); - ~GetResult(); + static GetResult make_for_tombstone(Timestamp removed_at_ts) { + return GetResult(removed_at_ts); + } + + ~GetResult() override; - Timestamp getTimestamp() const { return _timestamp; } + [[nodiscard]] Timestamp getTimestamp() const { + return _timestamp; + } - bool hasDocument() const { - return _doc.get() != NULL; + [[nodiscard]] bool hasDocument() const { + return (_doc.get() != nullptr); + } + + [[nodiscard]] bool is_tombstone() const noexcept { + return _is_tombstone; } const Document& getDocument() const { @@ -193,8 +210,12 @@ public: } private: + // Explicitly creates a tombstone (remove entry) GetResult with no document. + explicit GetResult(Timestamp removed_at_ts); + Timestamp _timestamp; DocumentSP _doc; + bool _is_tombstone; }; class BucketIdListResult : public Result { diff --git a/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp b/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp index 2927457a6a5..d6a3fea4735 100644 --- a/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp +++ b/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp @@ -686,6 +686,7 @@ TEST_F("require that get returns the first document found", SimpleFixture) { EXPECT_EQUAL(tstamp1, result.getTimestamp()); ASSERT_TRUE(result.hasDocument()); EXPECT_EQUAL(*doc1, result.getDocument()); + EXPECT_FALSE(result.is_tombstone()); } TEST_F("require that createIterator does", SimpleFixture) { diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp index 91ccac6fce1..1dc5199557a 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp @@ -434,7 +434,7 @@ PersistenceEngine::get(const Bucket& b, const document::FieldSet& fields, const search::DocumentMetaData meta = retriever.getDocumentMetaData(did); if (meta.timestamp != 0 && meta.bucketId == b.getBucketId()) { if (meta.removed) { - return GetResult(); + return GetResult::make_for_tombstone(meta.timestamp); } document::Document::UP doc = retriever.getDocument(meta.lid); if (!doc || doc->getId().getGlobalId() != meta.gid) { diff --git a/storage/src/vespa/storage/persistence/persistencethread.cpp b/storage/src/vespa/storage/persistence/persistencethread.cpp index d8f41008c8a..76c241d0627 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.cpp +++ b/storage/src/vespa/storage/persistence/persistencethread.cpp @@ -295,7 +295,8 @@ PersistenceThread::handleGet(api::GetCommand& cmd, MessageTracker::UP tracker) if (!result.hasDocument()) { _env._metrics.get[cmd.getLoadType()].notFound.inc(); } - tracker->setReply(std::make_shared(cmd, result.getDocumentPtr(), result.getTimestamp())); + tracker->setReply(std::make_shared(cmd, result.getDocumentPtr(), result.getTimestamp(), + false, result.is_tombstone())); } return tracker; -- cgit v1.2.3