Merge pull request #4974 from vespa-engine/toregge/handle-failure-to-get-bucket-list-due-to-old-storage-api-protocol

Handle failure to get bucket list due to old storage api protocol

4 files changed, 43 insertions, 3 deletions

diff --git a/storage/src/tests/distributor/bucketdbupdatertest.cpp b/storage/src/tests/distributor/bucketdbupdatertest.cpp
index ff442114c4c..b79894aee9a 100644
--- a/storage/src/tests/distributor/bucketdbupdatertest.cpp
+++ b/storage/src/tests/distributor/bucketdbupdatertest.cpp
@@ -34,6 +34,7 @@ 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);
@@ -92,6 +93,7 @@ protected:
     void testDistributorChangeWithGrouping();
     void testNormalUsageInitializing();
     void testFailedRequestBucketInfo();
+    void testEncodeErrorHandling();
     void testNoResponses();
     void testBitChange();
     void testInconsistentChecksum();
@@ -785,6 +787,29 @@ BucketDBUpdaterTest::testFailedRequestBucketInfo()
 }
 
 void
+BucketDBUpdaterTest::testEncodeErrorHandling()
+{
+    setSystemState(lib::ClusterState("distributor:1 .0.s:i storage:1"));
+
+    CPPUNIT_ASSERT_EQUAL(size_t(1), _sender.commands.size());
+
+    // Not yet passing on system state.
+    CPPUNIT_ASSERT_EQUAL(size_t(0), _senderDown.commands.size());
+    {
+        std::shared_ptr<api::RequestBucketInfoReply> reply =
+            getFakeBucketReply(lib::ClusterState("distributor:1 .0.s:i storage:1"),
+                               *((RequestBucketInfoCommand*)_sender.commands[0].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/vespa/storage/distributor/pendingclusterstate.cpp b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
index 51ef710d316..3c96bc55161 100644
--- a/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
+++ b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
@@ -232,7 +232,12 @@ PendingClusterState::onRequestBucketInfoReply(const std::shared_ptr<api::Request
     }
     const BucketSpaceAndNode bucketSpaceAndNode = iter->second;
 
-    if (!reply->getResult().success()) {
+    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.
+        LOG(debug, "Got ENCODE_ERROR, pretending success with no buckets");
+    } else if (!result.success()) {
         framework::MilliSecTime resendTime(_clock);
         resendTime += framework::MilliSecTime(100);
         _delayedRequests.emplace_back(resendTime, bucketSpaceAndNode);
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
index ece1445b18c..355a7871cc6 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
+++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
@@ -7,6 +7,7 @@
 #include <vespa/storageapi/message/bucketsplitting.h>
 #include <vespa/storageapi/message/multioperation.h>
 #include <vespa/document/bucket/fixed_bucket_spaces.h>
+#include <sstream>
 
 using document::BucketSpace;
 using document::FixedBucketSpaces;
@@ -25,7 +26,11 @@ void
 ProtocolSerialization5_0::putBucket(const document::Bucket& bucket, vespalib::GrowableByteBuffer& buf) const
 {
     buf.putLong(bucket.getBucketId().getRawId());
-    assert(bucket.getBucketSpace() == FixedBucketSpaces::default_space());
+    if (bucket.getBucketSpace() != FixedBucketSpaces::default_space()) {
+        std::ostringstream ost;
+        ost << "Bucket with bucket space " << bucket.getBucketSpace() << " cannot be serialized on old storageapi protocol.";
+        throw vespalib::IllegalArgumentException(ost.str(), VESPA_STRLOC);
+    }
 }
 
 document::BucketSpace
@@ -37,7 +42,11 @@ ProtocolSerialization5_0::getBucketSpace(document::ByteBuffer&) const
 void
 ProtocolSerialization5_0::putBucketSpace(document::BucketSpace bucketSpace, vespalib::GrowableByteBuffer&) const
 {
-    assert(bucketSpace == FixedBucketSpaces::default_space());
+    if (bucketSpace != FixedBucketSpaces::default_space()) {
+        std::ostringstream ost;
+        ost << "Bucket space " << bucketSpace << " cannot be serialized on old storageapi protocol.";
+        throw vespalib::IllegalArgumentException(ost.str(), VESPA_STRLOC);
+    }
 }
 
 api::BucketInfo
diff --git a/storageapi/src/vespa/storageapi/messageapi/returncode.h b/storageapi/src/vespa/storageapi/messageapi/returncode.h
index af4f14c1eeb..78f2168e170 100644
--- a/storageapi/src/vespa/storageapi/messageapi/returncode.h
+++ b/storageapi/src/vespa/storageapi/messageapi/returncode.h
@@ -30,6 +30,7 @@ public:
     /** Return status codes */
     enum Result {
         OK                            = mbus::ErrorCode::NONE,
+        ENCODE_ERROR                  = mbus::ErrorCode::ENCODE_ERROR,
 
         EXISTS                        = Protocol::ERROR_EXISTS,