diff options
Diffstat (limited to 'storageapi')
6 files changed, 72 insertions, 14 deletions
diff --git a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp index 0f628f59aac..37159ab0011 100644 --- a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp +++ b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp @@ -291,6 +291,7 @@ TEST_P(StorageProtocolTest, get) { EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId()); EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp()); EXPECT_EQ(Timestamp(100), reply2->getLastModifiedTimestamp()); + EXPECT_FALSE(reply2->is_tombstone()); EXPECT_NO_FATAL_FAILURE(assert_bucket_info_reply_fields_propagated(*reply2)); } @@ -316,6 +317,40 @@ TEST_P(StorageProtocolTest, can_set_internal_read_consistency_on_get_commands) { EXPECT_EQ(cmd2->internal_read_consistency(), InternalReadConsistency::Strong); } +TEST_P(StorageProtocolTest, tombstones_propagated_for_gets) { + // Only supported on protocol version 7+. + if (GetParam().getMajor() < 7) { + return; + } + auto cmd = std::make_shared<GetCommand>(_bucket, _testDocId, "foo,bar", 123); + auto reply = std::make_shared<GetReply>(*cmd, std::shared_ptr<Document>(), 100, false, true); + set_dummy_bucket_info_reply_fields(*reply); + auto reply2 = copyReply(reply); + + EXPECT_TRUE(reply2->getDocument().get() == nullptr); + EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId()); + EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp()); + EXPECT_EQ(Timestamp(100), reply2->getLastModifiedTimestamp()); // In this case, the tombstone timestamp. + EXPECT_TRUE(reply2->is_tombstone()); +} + +// TODO remove this once pre-protobuf serialization is removed +TEST_P(StorageProtocolTest, old_serialization_format_treats_tombstone_get_replies_as_not_found) { + if (GetParam().getMajor() >= 7) { + return; + } + auto cmd = std::make_shared<GetCommand>(_bucket, _testDocId, "foo,bar", 123); + auto reply = std::make_shared<GetReply>(*cmd, std::shared_ptr<Document>(), 100, false, true); + set_dummy_bucket_info_reply_fields(*reply); + auto reply2 = copyReply(reply); + + EXPECT_TRUE(reply2->getDocument().get() == nullptr); + EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId()); + EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp()); + EXPECT_EQ(Timestamp(0), reply2->getLastModifiedTimestamp()); + EXPECT_FALSE(reply2->is_tombstone()); // Protocol version doesn't understand explicit tombstones. +} + TEST_P(StorageProtocolTest, remove) { auto cmd = std::make_shared<RemoveCommand>(_bucket, _testDocId, 159); auto cmd2 = copyCommand(cmd); diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto index 12dbaf59146..f38c5bfb56a 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto +++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto @@ -73,6 +73,9 @@ message GetResponse { uint64 last_modified_timestamp = 2; BucketInfo bucket_info = 3; BucketId remapped_bucket_id = 4; + // Note: last_modified_timestamp and tombstone_timestamp are mutually exclusive. + // Tracked separately (rather than being a flag bool) to avoid issues during rolling upgrades. + uint64 tombstone_timestamp = 5; } message RevertRequest { diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp index aeb6d382997..8a0204c8fc9 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp @@ -168,7 +168,8 @@ ProtocolSerialization5_0::onDecodeUpdateReply(const SCmd& cmd, BBuf& buf) const void ProtocolSerialization5_0::onEncode(GBBuf& buf, const api::GetReply& msg) const { SH::putDocument(msg.getDocument().get(), buf); - buf.putLong(msg.getLastModifiedTimestamp()); + // Old protocol version doesn't understand tombstones. Make it appear as Not Found. + buf.putLong(msg.is_tombstone() ? api::Timestamp(0) : msg.getLastModifiedTimestamp()); onEncodeBucketInfoReply(buf, msg); } diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp index 90c8d1c7d2a..8ea946eede4 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp @@ -567,7 +567,17 @@ void ProtocolSerialization7::onEncode(GBBuf& buf, const api::GetReply& msg) cons if (msg.getDocument()) { set_document(*res.mutable_document(), *msg.getDocument()); } - res.set_last_modified_timestamp(msg.getLastModifiedTimestamp()); + if (!msg.is_tombstone()) { + res.set_last_modified_timestamp(msg.getLastModifiedTimestamp()); + } else { + // This field will be ignored by older versions, making the behavior as if + // a timestamp of zero was returned for tombstones, as it the legacy behavior. + res.set_tombstone_timestamp(msg.getLastModifiedTimestamp()); + // Will not be encoded onto the wire, but we include it here to hammer down the + // point that it's intentional to have the last modified time appear as a not + // found document for older versions. + res.set_last_modified_timestamp(0); + } }); } @@ -585,8 +595,12 @@ api::StorageReply::UP ProtocolSerialization7::onDecodeGetReply(const SCmd& cmd, return decode_bucket_info_response<protobuf::GetResponse>(buf, [&](auto& res) { try { auto document = get_document(res.document(), type_repo()); + const bool is_tombstone = (res.tombstone_timestamp() != 0); + const auto effective_timestamp = (is_tombstone ? res.tombstone_timestamp() + : res.last_modified_timestamp()); return std::make_unique<api::GetReply>(static_cast<const api::GetCommand&>(cmd), - std::move(document), res.last_modified_timestamp()); + std::move(document), effective_timestamp, + false, is_tombstone); } catch (std::exception& e) { auto reply = std::make_unique<api::GetReply>(static_cast<const api::GetCommand&>(cmd), std::shared_ptr<document::Document>(), 0u); diff --git a/storageapi/src/vespa/storageapi/message/persistence.cpp b/storageapi/src/vespa/storageapi/message/persistence.cpp index 7fd789a8c81..af0b7ae0288 100644 --- a/storageapi/src/vespa/storageapi/message/persistence.cpp +++ b/storageapi/src/vespa/storageapi/message/persistence.cpp @@ -211,14 +211,16 @@ GetCommand::print(std::ostream& out, bool verbose, const std::string& indent) co GetReply::GetReply(const GetCommand& cmd, const DocumentSP& doc, Timestamp lastModified, - bool had_consistent_replicas) + bool had_consistent_replicas, + bool is_tombstone) : BucketInfoReply(cmd), _docId(cmd.getDocumentId()), _fieldSet(cmd.getFieldSet()), _doc(doc), _beforeTimestamp(cmd.getBeforeTimestamp()), _lastModifiedTime(lastModified), - _had_consistent_replicas(had_consistent_replicas) + _had_consistent_replicas(had_consistent_replicas), + _is_tombstone(is_tombstone) { } diff --git a/storageapi/src/vespa/storageapi/message/persistence.h b/storageapi/src/vespa/storageapi/message/persistence.h index e6fe2b6dae5..24601803266 100644 --- a/storageapi/src/vespa/storageapi/message/persistence.h +++ b/storageapi/src/vespa/storageapi/message/persistence.h @@ -224,24 +224,27 @@ class GetReply : public BucketInfoReply { Timestamp _beforeTimestamp; Timestamp _lastModifiedTime; bool _had_consistent_replicas; - + bool _is_tombstone; public: - GetReply(const GetCommand& cmd, - const DocumentSP& doc = DocumentSP(), - Timestamp lastModified = 0, - bool had_consistent_replicas = false); + explicit GetReply(const GetCommand& cmd, + const DocumentSP& doc = DocumentSP(), + Timestamp lastModified = 0, + bool had_consistent_replicas = false, + bool is_tombstone = false); + ~GetReply() override; const DocumentSP& getDocument() const { return _doc; } const document::DocumentId& getDocumentId() const { return _docId; } const vespalib::string& getFieldSet() const { return _fieldSet; } - Timestamp getLastModifiedTimestamp() const { return _lastModifiedTime; } - Timestamp getBeforeTimestamp() const { return _beforeTimestamp; } + Timestamp getLastModifiedTimestamp() const noexcept { return _lastModifiedTime; } + Timestamp getBeforeTimestamp() const noexcept { return _beforeTimestamp; } - bool had_consistent_replicas() const noexcept { return _had_consistent_replicas; } + [[nodiscard]] bool had_consistent_replicas() const noexcept { return _had_consistent_replicas; } + [[nodiscard]] bool is_tombstone() const noexcept { return _is_tombstone; } - bool wasFound() const { return (_doc.get() != 0); } + bool wasFound() const { return (_doc.get() != nullptr); } void print(std::ostream& out, bool verbose, const std::string& indent) const override; DECLARE_STORAGEREPLY(GetReply, onGetReply) }; |