Remove workarounds added during transition to multiple bucket spaces.

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));