diff options
-rw-r--r-- | storage/src/tests/persistence/filestorage/sanitycheckeddeletetest.cpp | 58 | ||||
-rw-r--r-- | storage/src/vespa/storage/persistence/persistencethread.cpp | 17 |
2 files changed, 57 insertions, 18 deletions
diff --git a/storage/src/tests/persistence/filestorage/sanitycheckeddeletetest.cpp b/storage/src/tests/persistence/filestorage/sanitycheckeddeletetest.cpp index 19841e98e85..363c89165b3 100644 --- a/storage/src/tests/persistence/filestorage/sanitycheckeddeletetest.cpp +++ b/storage/src/tests/persistence/filestorage/sanitycheckeddeletetest.cpp @@ -9,34 +9,29 @@ namespace storage { -class SanityCheckedDeleteTest : public FileStorTestFixture -{ +class SanityCheckedDeleteTest : public FileStorTestFixture { public: - void testDeleteBucketFailsWhenProviderOutOfSync(); + void delete_bucket_fails_when_provider_out_of_sync(); + void differing_document_sizes_not_considered_out_of_sync(); CPPUNIT_TEST_SUITE(SanityCheckedDeleteTest); - CPPUNIT_TEST(testDeleteBucketFailsWhenProviderOutOfSync); + CPPUNIT_TEST(delete_bucket_fails_when_provider_out_of_sync); + CPPUNIT_TEST(differing_document_sizes_not_considered_out_of_sync); CPPUNIT_TEST_SUITE_END(); + + spi::BucketInfo send_put_and_get_bucket_info(TestFileStorComponents &c, const spi::Bucket &spiBucket); }; CPPUNIT_TEST_SUITE_REGISTRATION(SanityCheckedDeleteTest); -void -SanityCheckedDeleteTest::testDeleteBucketFailsWhenProviderOutOfSync() -{ - TestFileStorComponents c(*this, "testDeleteBucketFailsWhenProviderOutOfSync"); +void SanityCheckedDeleteTest::delete_bucket_fails_when_provider_out_of_sync() { + TestFileStorComponents c(*this, "delete_bucket_fails_when_provider_out_of_sync"); document::BucketId bucket(8, 123); document::BucketId syncBucket(8, 234); spi::Bucket spiBucket(bucket, spi::PartitionId(0)); - createBucket(bucket); // Send a put to ensure bucket isn't empty. - c.sendPut(bucket, DocumentIndex(0), PutTimestamp(1000)); - c.top.waitForMessages(1, MSG_WAIT_TIME); - c.top.getRepliesOnce(); - spi::BucketInfo infoBefore( - _node->getPersistenceProvider() - .getBucketInfo(spiBucket).getBucketInfo()); + spi::BucketInfo infoBefore(send_put_and_get_bucket_info(c, spiBucket)); createBucket(syncBucket); @@ -50,8 +45,7 @@ SanityCheckedDeleteTest::testDeleteBucketFailsWhenProviderOutOfSync() entry.write(); } - std::shared_ptr<api::DeleteBucketCommand> cmd( - new api::DeleteBucketCommand(bucket)); + auto cmd = std::make_shared<api::DeleteBucketCommand>(bucket); cmd->setBucketInfo(serviceLayerInfo); c.top.sendDown(cmd); @@ -75,4 +69,34 @@ SanityCheckedDeleteTest::testDeleteBucketFailsWhenProviderOutOfSync() CPPUNIT_ASSERT(infoBefore == infoResult.getBucketInfo()); } +spi::BucketInfo SanityCheckedDeleteTest::send_put_and_get_bucket_info( + FileStorTestFixture::TestFileStorComponents& c, + const spi::Bucket& spiBucket) { + createBucket(spiBucket.getBucketId()); + c.sendPut(spiBucket.getBucketId(), DocumentIndex(0), PutTimestamp(1000)); + c.top.waitForMessages(1, MSG_WAIT_TIME); + c.top.getRepliesOnce(); + return _node->getPersistenceProvider().getBucketInfo(spiBucket).getBucketInfo(); +} + +void SanityCheckedDeleteTest::differing_document_sizes_not_considered_out_of_sync() { + TestFileStorComponents c(*this, "differing_document_sizes_not_considered_out_of_sync"); + document::BucketId bucket(8, 123); + spi::Bucket spiBucket(bucket, spi::PartitionId(0)); + + spi::BucketInfo info_before(send_put_and_get_bucket_info(c, spiBucket)); + // Expect 1 byte of reported size, which will mismatch with the actually put document. + api::BucketInfo info_with_size_diff(info_before.getChecksum(), info_before.getDocumentCount(), 1u); + + auto delete_cmd = std::make_shared<api::DeleteBucketCommand>(bucket); + delete_cmd->setBucketInfo(info_with_size_diff); + + c.top.sendDown(delete_cmd); + c.top.waitForMessages(1, MSG_WAIT_TIME); + // Bucket should now well and truly be gone. Will trigger a getBucketInfo error response. + spi::BucketInfoResult info_post_delete( + _node->getPersistenceProvider().getBucketInfo(spiBucket)); + CPPUNIT_ASSERT_MSG(info_post_delete.getErrorMessage(), info_post_delete.hasError()); +} + } // namespace storage diff --git a/storage/src/vespa/storage/persistence/persistencethread.cpp b/storage/src/vespa/storage/persistence/persistencethread.cpp index d2bfcccb8cf..f0ff2bafdb2 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.cpp +++ b/storage/src/vespa/storage/persistence/persistencethread.cpp @@ -343,6 +343,21 @@ PersistenceThread::handleCreateBucket(api::CreateBucketCommand& cmd) return tracker; } +namespace { + +bool bucketStatesAreSemanticallyEqual(const api::BucketInfo& a, const api::BucketInfo& b) { + // Don't check document sizes, as background moving of documents in Proton + // may trigger a change in size without any mutations taking place. This will + // only take place when a document being moved was fed _prior_ to the change + // where Proton starts reporting actual document sizes, and will eventually + // converge to a stable value. But for now, ignore it to prevent false positive + // error logs and non-deleted buckets. + return ((a.getChecksum() == b.getChecksum()) + && (a.getDocumentCount() == b.getDocumentCount())); +} + +} + bool PersistenceThread::checkProviderBucketInfoMatches(const spi::Bucket& bucket, const api::BucketInfo& info) const @@ -361,7 +376,7 @@ PersistenceThread::checkProviderBucketInfoMatches(const spi::Bucket& bucket, // that important and ready may change under the hood in a race with // getModifiedBuckets(). If bucket is empty it means it has already // been deleted by a racing split/join. - if (!info.equalDocumentInfo(providerInfo) && !providerInfo.empty()) { + if (!bucketStatesAreSemanticallyEqual(info, providerInfo) && !providerInfo.empty()) { LOG(error, "Service layer bucket database and provider out of sync before " "deleting bucket %s! Service layer db had %s while provider says " |