diff options
author | Tor Egge <Tor.Egge@broadpark.no> | 2018-09-24 16:14:32 +0200 |
---|---|---|
committer | gjoranv <gv@oath.com> | 2019-01-21 15:09:22 +0100 |
commit | abdd74bab2a9c834e6b13bd88270896e2f59af1f (patch) | |
tree | 79686117ae02a5a6463ce08d38a8a93ad8e4728e /storage/src | |
parent | 3c56e9fc27d245fa7c489c689a56eb1e5bdea117 (diff) |
Remove workarounds added during transition to multiple bucket spaces.
Diffstat (limited to 'storage/src')
4 files changed, 9 insertions, 51 deletions
diff --git a/storage/src/tests/distributor/bucketdbupdatertest.cpp b/storage/src/tests/distributor/bucketdbupdatertest.cpp index 56f88b7f98f..53f80854bef 100644 --- a/storage/src/tests/distributor/bucketdbupdatertest.cpp +++ b/storage/src/tests/distributor/bucketdbupdatertest.cpp @@ -61,7 +61,6 @@ class BucketDBUpdaterTest : public CppUnit::TestFixture, CPPUNIT_TEST(testDistributorChangeWithGrouping); CPPUNIT_TEST(testNormalUsageInitializing); // Check that we send request bucket info when storage node is initializing, and send another when it's up. CPPUNIT_TEST(testFailedRequestBucketInfo); - CPPUNIT_TEST(testEncodeErrorHandling); CPPUNIT_TEST(testBitChange); // Check what happens when distribution bits change CPPUNIT_TEST(testNodeDown); CPPUNIT_TEST(testStorageNodeInMaintenanceClearsBucketsForNode); @@ -123,7 +122,6 @@ protected: void testDistributorChangeWithGrouping(); void testNormalUsageInitializing(); void testFailedRequestBucketInfo(); - void testEncodeErrorHandling(); void testNoResponses(); void testBitChange(); void testInconsistentChecksum(); @@ -811,29 +809,6 @@ BucketDBUpdaterTest::testFailedRequestBucketInfo() } void -BucketDBUpdaterTest::testEncodeErrorHandling() -{ - setSystemState(lib::ClusterState("distributor:1 .0.s:i storage:1")); - - CPPUNIT_ASSERT_EQUAL(_bucketSpaces.size(), _sender.commands.size()); - - // Not yet passing on system state. - CPPUNIT_ASSERT_EQUAL(size_t(0), _senderDown.commands.size()); - for (uint32_t i = 0; i < _bucketSpaces.size(); ++i) { - std::shared_ptr<api::RequestBucketInfoReply> reply = - getFakeBucketReply(lib::ClusterState("distributor:1 .0.s:i storage:1"), - *((RequestBucketInfoCommand*)_sender.commands[i].get()), - 0, - 10); - - reply->setResult(api::ReturnCode::ENCODE_ERROR); - getBucketDBUpdater().onRequestBucketInfoReply(reply); - } - CPPUNIT_ASSERT_EQUAL(std::string("Set system state"), - _senderDown.getCommands()); -} - -void BucketDBUpdaterTest::testDownWhileInit() { setStorageNodes(3); diff --git a/storage/src/tests/storageserver/communicationmanagertest.cpp b/storage/src/tests/storageserver/communicationmanagertest.cpp index 3b72585b076..6af2733f3b9 100644 --- a/storage/src/tests/storageserver/communicationmanagertest.cpp +++ b/storage/src/tests/storageserver/communicationmanagertest.cpp @@ -29,7 +29,7 @@ struct CommunicationManagerTest : public CppUnit::TestFixture { void testRepliesAreDequeuedInFifoOrder(); void bucket_space_config_can_be_updated_live(); void unmapped_bucket_space_documentapi_request_returns_error_reply(); - void unmapped_bucket_space_for_get_documentapi_request_returns_empty_reply(); + void unmapped_bucket_space_for_get_documentapi_request_returns_error_reply(); static constexpr uint32_t MESSAGE_WAIT_TIME_SEC = 60; @@ -54,7 +54,7 @@ struct CommunicationManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testRepliesAreDequeuedInFifoOrder); CPPUNIT_TEST(bucket_space_config_can_be_updated_live); CPPUNIT_TEST(unmapped_bucket_space_documentapi_request_returns_error_reply); - CPPUNIT_TEST(unmapped_bucket_space_for_get_documentapi_request_returns_empty_reply); + CPPUNIT_TEST(unmapped_bucket_space_for_get_documentapi_request_returns_error_reply); CPPUNIT_TEST_SUITE_END(); }; @@ -349,11 +349,7 @@ void CommunicationManagerTest::unmapped_bucket_space_documentapi_request_returns CPPUNIT_ASSERT_EQUAL(uint64_t(1), f.comm_mgr->metrics().bucketSpaceMappingFailures.getValue()); } -// Legacy DocumentAPI routing protocols will send Gets to _all_ clusters even -// if they do not contain a particular document type. By sending an empty reply -// we signal a mergeable "not found" to the sender rather than a non-mergeable -// fatal error. -void CommunicationManagerTest::unmapped_bucket_space_for_get_documentapi_request_returns_empty_reply() { +void CommunicationManagerTest::unmapped_bucket_space_for_get_documentapi_request_returns_error_reply() { CommunicationManagerFixture f; BucketspacesConfigBuilder config; @@ -363,9 +359,9 @@ void CommunicationManagerTest::unmapped_bucket_space_for_get_documentapi_request f.comm_mgr->handleMessage(f.documentapi_get_message_for_space("fluff")); CPPUNIT_ASSERT_EQUAL(size_t(1), f.reply_handler.replies.size()); auto& reply = *f.reply_handler.replies[0]; - CPPUNIT_ASSERT(!reply.hasErrors()); - auto& get_reply = dynamic_cast<documentapi::GetDocumentReply&>(reply); - CPPUNIT_ASSERT(!get_reply.hasDocument()); + CPPUNIT_ASSERT(reply.hasErrors()); + CPPUNIT_ASSERT_EQUAL(static_cast<uint32_t>(api::ReturnCode::REJECTED), reply.getError(0).getCode()); + CPPUNIT_ASSERT_EQUAL(uint64_t(1), f.comm_mgr->metrics().bucketSpaceMappingFailures.getValue()); } } // storage diff --git a/storage/src/vespa/storage/distributor/pendingclusterstate.cpp b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp index 471ce7e2b27..1996ae9d2af 100644 --- a/storage/src/vespa/storage/distributor/pendingclusterstate.cpp +++ b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp @@ -233,12 +233,7 @@ PendingClusterState::onRequestBucketInfoReply(const std::shared_ptr<api::Request const BucketSpaceAndNode bucketSpaceAndNode = iter->second; api::ReturnCode result(reply->getResult()); - if (result == api::ReturnCode::Result::ENCODE_ERROR) { - // Handle failure to encode bucket space due to use of old storage api - // protocol. Pretend that request succeeded with no buckets returned. - // TODO remove this workaround for Vespa 7 - LOG(debug, "Got ENCODE_ERROR, pretending success with no buckets"); - } else if (!result.success()) { + if (!result.success()) { framework::MilliSecTime resendTime(_clock); resendTime += framework::MilliSecTime(100); _delayedRequests.emplace_back(resendTime, bucketSpaceAndNode); diff --git a/storage/src/vespa/storage/storageserver/communicationmanager.cpp b/storage/src/vespa/storage/storageserver/communicationmanager.cpp index 5a1c2aed113..7fb85ef0ecc 100644 --- a/storage/src/vespa/storage/storageserver/communicationmanager.cpp +++ b/storage/src/vespa/storage/storageserver/communicationmanager.cpp @@ -259,16 +259,8 @@ void CommunicationManager::fail_with_unresolvable_bucket_space( LOG(debug, "Could not map DocumentAPI message to internal bucket: %s", error_message.c_str()); MBUS_TRACE(msg->getTrace(), 6, "Communication manager: Failing message as its document type has no known bucket space mapping"); std::unique_ptr<mbus::Reply> reply; - if (msg->getType() == documentapi::DocumentProtocol::MESSAGE_GETDOCUMENT) { - // HACK: to avoid breaking legacy routing of GetDocumentMessages to _all_ clusters - // regardless of them having a document type or not, we remap missing bucket spaces - // to explicit Not Found replies (empty document GetDocumentReply). - // TODO remove this workaround for Vespa 7 - reply = std::make_unique<documentapi::GetDocumentReply>(std::shared_ptr<document::Document>()); - } else { - reply = std::make_unique<mbus::EmptyReply>(); - reply->addError(mbus::Error(documentapi::DocumentProtocol::ERROR_REJECTED, error_message)); - } + reply = std::make_unique<mbus::EmptyReply>(); + reply->addError(mbus::Error(documentapi::DocumentProtocol::ERROR_REJECTED, error_message)); msg->swapState(*reply); _metrics.bucketSpaceMappingFailures.inc(); _messageBusSession->reply(std::move(reply)); |