Revert "Implicitly enable multiple bucket spaces"

8 files changed, 149 insertions, 36 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/GlobalDistributionValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/content/GlobalDistributionValidator.java
index 0661ef67131..4bdef0607a2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/GlobalDistributionValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/GlobalDistributionValidator.java
@@ -21,11 +21,45 @@ import static java.util.stream.Collectors.toSet;
 public class GlobalDistributionValidator {
 
     public void validate(Map<String, NewDocumentType> documentDefinitions,
-                         Set<NewDocumentType> globallyDistributedDocuments) {
+                         Set<NewDocumentType> globallyDistributedDocuments,
+                         Redundancy redundancy,
+                         boolean enableMultipleBucketSpaces) {
+        if (!enableMultipleBucketSpaces) {
+            verifyGlobalDocumentsHaveRequiredRedundancy(globallyDistributedDocuments, redundancy);
+            verifySearchableCopiesIsSameAsRedundancy(globallyDistributedDocuments, redundancy);
+        }
         verifyReferredDocumentsArePresent(documentDefinitions);
         verifyReferredDocumentsAreGlobal(documentDefinitions, globallyDistributedDocuments);
     }
 
+    private static void verifyGlobalDocumentsHaveRequiredRedundancy(Set<NewDocumentType> globallyDistributedDocuments,
+                                                                    Redundancy redundancy) {
+        if (!globallyDistributedDocuments.isEmpty() && !redundancy.isEffectivelyGloballyDistributed()) {
+            throw new IllegalArgumentException(
+                    String.format(
+                            "The following document types are marked as global, " +
+                                    "but do not have high enough redundancy to make the documents globally distributed: %s. " +
+                                    "Redundancy is %d, expected %d.",
+                            asPrintableString(toDocumentNameStream(globallyDistributedDocuments)),
+                            redundancy.effectiveFinalRedundancy(),
+                            redundancy.totalNodes()));
+        }
+    }
+
+    private static void verifySearchableCopiesIsSameAsRedundancy(Set<NewDocumentType> globallyDistributedDocuments,
+                                                                 Redundancy redundancy) {
+        if (!globallyDistributedDocuments.isEmpty() &&
+                redundancy.effectiveReadyCopies() != redundancy.effectiveFinalRedundancy()) {
+            throw new IllegalArgumentException(
+                    String.format(
+                            "The following document types have the number of searchable copies less than redundancy: %s. " +
+                                    "Searchable copies is %d, while redundancy is %d.",
+                            asPrintableString(toDocumentNameStream(globallyDistributedDocuments)),
+                            redundancy.effectiveReadyCopies(),
+                            redundancy.effectiveFinalRedundancy()));
+        }
+    }
+
     private static void verifyReferredDocumentsArePresent(Map<String, NewDocumentType> documentDefinitions) {
         Set<NewDocumentType.Name> unknowDocuments = getReferencedDocuments(documentDefinitions)
                 .filter(name -> !documentDefinitions.containsKey(name.toString()))
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
index 68ae4d2b242..0119bced095 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
@@ -61,12 +61,14 @@ public class ContentCluster extends AbstractConfigProducer implements
                                                            MessagetyperouteselectorpolicyConfig.Producer,
                                                            BucketspacesConfig.Producer {
 
+    // TODO: Make private
     private String documentSelection;
     private ContentSearchCluster search;
     private final boolean isHostedVespa;
     private final Map<String, NewDocumentType> documentDefinitions;
     private final Set<NewDocumentType> globallyDistributedDocuments;
-    private boolean forceEnableMultipleBucketSpaces = false;
+    // Experimental flag (TODO: remove when feature is enabled by default)
+    private boolean enableMultipleBucketSpaces = false;
     private com.yahoo.vespa.model.content.StorageGroup rootGroup;
     private StorageCluster storageNodes;
     private DistributorCluster distributorNodes;
@@ -248,7 +250,7 @@ public class ContentCluster extends AbstractConfigProducer implements
         private void setupExperimental(ContentCluster cluster, ModelElement experimental) {
             Boolean enableMultipleBucketSpaces = experimental.childAsBoolean("enable-multiple-bucket-spaces");
             if (enableMultipleBucketSpaces != null) {
-                cluster.forceEnableMultipleBucketSpaces = enableMultipleBucketSpaces;
+                cluster.enableMultipleBucketSpaces = enableMultipleBucketSpaces;
             }
         }
 
@@ -594,13 +596,13 @@ public class ContentCluster extends AbstractConfigProducer implements
             builder.min_distributor_up_ratio(0);
             builder.min_storage_up_ratio(0);
         }
-        builder.enable_multiple_bucket_spaces(true);
+        builder.enable_multiple_bucket_spaces(enableMultipleBucketSpaces);
         // Telling the controller whether we actually _have_ global document types lets
         // it selectively enable or disable constraints that aren't needed when these
         // are not are present, even if full protocol and backend support is enabled
         // for multiple bucket spaces. Basically, if you don't use it, you don't
         // pay for it.
-        builder.cluster_has_global_document_types(!globallyDistributedDocuments.isEmpty());
+        builder.cluster_has_global_document_types(enableMultipleBucketSpaces && !globallyDistributedDocuments.isEmpty());
     }
 
     @Override
@@ -644,7 +646,7 @@ public class ContentCluster extends AbstractConfigProducer implements
             }
         }
         new ReservedDocumentTypeNameValidator().validate(documentDefinitions);
-        new GlobalDistributionValidator().validate(documentDefinitions, globallyDistributedDocuments);
+        new GlobalDistributionValidator().validate(documentDefinitions, globallyDistributedDocuments, redundancy, enableMultipleBucketSpaces);
     }
 
     public static Map<String, Integer> METRIC_INDEX_MAP = new TreeMap<>();
@@ -725,13 +727,11 @@ public class ContentCluster extends AbstractConfigProducer implements
         for (NewDocumentType docType : getDocumentDefinitions().values()) {
             BucketspacesConfig.Documenttype.Builder docTypeBuilder = new BucketspacesConfig.Documenttype.Builder();
             docTypeBuilder.name(docType.getName());
-            String bucketSpace = (isGloballyDistributed(docType) ? GLOBAL_BUCKET_SPACE : DEFAULT_BUCKET_SPACE);
+            String bucketSpace = ((enableMultipleBucketSpaces && isGloballyDistributed(docType))
+                    ? GLOBAL_BUCKET_SPACE : DEFAULT_BUCKET_SPACE);
             docTypeBuilder.bucketspace(bucketSpace);
             builder.documenttype(docTypeBuilder);
         }
-        // NOTE: this config is kept around to allow the use of multiple bucket spaces
-        // on older versions of Vespa. It is for all intents and purposes a no-op in
-        // newer versions where multiple bucket spaces are enabled by default.
-        builder.enable_multiple_bucket_spaces(forceEnableMultipleBucketSpaces);
+        builder.enable_multiple_bucket_spaces(enableMultipleBucketSpaces);
     }
 }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/ContentSearchClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/ContentSearchClusterTest.java
index 0156128f7ca..98177b4ada0 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/ContentSearchClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/ContentSearchClusterTest.java
@@ -173,17 +173,17 @@ public class ContentSearchClusterTest {
     }
 
     @Test
-    public void require_that_document_types_belong_to_correct_bucket_spaces() throws Exception {
+    public void require_that_all_document_types_belong_to_default_bucket_space_by_default() throws Exception {
         BucketspacesConfig config = getBucketspacesConfig(createClusterWithGlobalType());
         assertEquals(2, config.documenttype().size());
-        assertDocumentType("global", "global", config.documenttype(0));
+        assertDocumentType("global", "default", config.documenttype(0));
         assertDocumentType("regular", "default", config.documenttype(1));
         // Safeguard against flipping the switch
         assertFalse(config.enable_multiple_bucket_spaces());
     }
 
     @Test
-    public void require_that_multiple_bucket_spaces_can_be_force_enabled() throws Exception {
+    public void require_that_multiple_bucket_spaces_can_be_enabled() throws Exception {
         ContentCluster cluster = createClusterWithMultipleBucketSpacesEnabled();
         {
             BucketspacesConfig config = getBucketspacesConfig(cluster);
@@ -210,9 +210,9 @@ public class ContentSearchClusterTest {
     }
 
     @Test
-    public void controller_global_documents_config_always_enabled_even_without_experimental_flag_set() throws Exception {
+    public void controller_global_documents_config_forced_to_false_if_multiple_spaces_not_enabled() throws Exception {
         ContentCluster cluster = createClusterWithGlobalDocsButNotMultipleSpacesEnabled();
-        assertTrue(getFleetcontrollerConfig(cluster).cluster_has_global_document_types());
+        assertFalse(getFleetcontrollerConfig(cluster).cluster_has_global_document_types());
     }
 
 }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/GlobalDistributionValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/GlobalDistributionValidatorTest.java
index 6506f7a08a8..b8252f2f081 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/GlobalDistributionValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/GlobalDistributionValidatorTest.java
@@ -26,16 +26,64 @@ public class GlobalDistributionValidatorTest {
     public final ExpectedException exceptionRule = ExpectedException.none();
 
     @Test
+    public void throws_exception_if_redudancy_does_not_imply_global_distribution() {
+        Fixture fixture = new Fixture()
+                .addGlobalDocument(createDocumentType("foo"))
+                .addGlobalDocument(createDocumentType("bar"));
+        Redundancy redundancy = createRedundancyWithoutGlobalDistribution();
+
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage(
+                "The following document types are marked as global, " +
+                        "but do not have high enough redundancy to make the documents globally distributed: " +
+                        "'bar', 'foo'. Redundancy is 2, expected 3.");
+        validate(fixture, redundancy);
+    }
+
+    @Test
+    public void validation_of_redundancy_is_deactivated_if_multiple_bucket_spaces_is_enabled() {
+        Fixture fixture = new Fixture()
+                .addGlobalDocument(createDocumentType("foo"))
+                .addGlobalDocument(createDocumentType("bar"));
+        Redundancy redundancy = createRedundancyWithoutGlobalDistributionAndTooFewSearchableCopies();
+
+        validate(fixture, redundancy, true);
+    }
+
+    @Test
+    public void throws_exception_if_searchable_copies_too_low() {
+        Fixture fixture = new Fixture()
+                .addGlobalDocument(createDocumentType("foo"))
+                .addGlobalDocument(createDocumentType("bar"));
+        Redundancy redundancy = createRedundancyWithTooFewSearchableCopies();
+
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage(
+                "The following document types have the number of searchable copies less than redundancy: " +
+                "'bar', 'foo'. Searchable copies is 1, while redundancy is 2.");
+        validate(fixture, redundancy);
+    }
+
+    @Test
+    public void validation_succeeds_when_globally_distributed_and_enough_searchable_copies() {
+        Fixture fixture = new Fixture()
+                .addGlobalDocument(createDocumentType("foo"));
+        Redundancy redundancy = createRedundancyWithGlobalDistribution();
+        validate(fixture, redundancy);
+    }
+
+    @Test
     public void validation_succeeds_on_no_documents() {
         new GlobalDistributionValidator()
-                .validate(emptyMap(), emptySet());
+                .validate(emptyMap(), emptySet(), createRedundancyWithoutGlobalDistribution(), false);
     }
 
     @Test
     public void validation_succeeds_on_no_global_documents() {
         Fixture fixture = new Fixture()
                 .addNonGlobalDocument(createDocumentType("foo"));
-        validate(fixture);
+        Redundancy redundancy = createRedundancyWithoutGlobalDistribution();
+        validate(fixture, redundancy);
     }
 
     @Test
@@ -44,10 +92,11 @@ public class GlobalDistributionValidatorTest {
         Fixture fixture = new Fixture()
                 .addNonGlobalDocument(parent)
                 .addNonGlobalDocument(createDocumentType("child", parent));
+        Redundancy redundancy = createRedundancyWithoutGlobalDistribution();
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage(
                 "The following document types are referenced from other documents, but are not globally distributed: 'parent'");
-        validate(fixture);
+        validate(fixture, redundancy);
     }
 
     @Test
@@ -56,7 +105,8 @@ public class GlobalDistributionValidatorTest {
         Fixture fixture = new Fixture()
                 .addGlobalDocument(parent)
                 .addNonGlobalDocument(createDocumentType("child", parent));
-        validate(fixture);
+        Redundancy redundancy = createRedundancyWithGlobalDistribution();
+        validate(fixture, redundancy);
     }
 
     @Test
@@ -65,10 +115,11 @@ public class GlobalDistributionValidatorTest {
         NewDocumentType child = createDocumentType("child", unknown);
         Fixture fixture = new Fixture()
                 .addNonGlobalDocument(child);
+        Redundancy redundancy = createRedundancyWithGlobalDistribution();
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage(
                 "The following document types are referenced from other documents, but are not listed in services.xml: 'unknown'");
-        validate(fixture);
+        validate(fixture, redundancy);
     }
 
     @Test
@@ -79,14 +130,42 @@ public class GlobalDistributionValidatorTest {
         new VespaModelCreatorWithFilePkg("src/test/cfg/application/validation/global_distribution_validation/").create();
     }
 
+    private static Redundancy createRedundancyWithGlobalDistribution() {
+        Redundancy redundancy = new Redundancy(2, 2, 2);
+        redundancy.setTotalNodes(2);
+        return redundancy;
+    }
+
+    private static Redundancy createRedundancyWithoutGlobalDistribution() {
+        Redundancy redundancy = new Redundancy(2, 2, 2);
+        redundancy.setTotalNodes(3);
+        return redundancy;
+    }
+
+    private static Redundancy createRedundancyWithTooFewSearchableCopies() {
+        Redundancy redundancy = new Redundancy(2, 2, 1);
+        redundancy.setTotalNodes(2);
+        return redundancy;
+    }
+
+    private static Redundancy createRedundancyWithoutGlobalDistributionAndTooFewSearchableCopies() {
+        Redundancy redundancy = new Redundancy(2, 2, 1);
+        redundancy.setTotalNodes(3);
+        return redundancy;
+    }
+
     private static NewDocumentType createDocumentType(String name, NewDocumentType... references) {
         Set<NewDocumentType.Name> documentReferences = Stream.of(references).map(NewDocumentType::getFullName).collect(toSet());
         return new NewDocumentType(new NewDocumentType.Name(name), documentReferences);
     }
 
-    private static void validate(Fixture fixture) {
+    private static void validate(Fixture fixture, Redundancy redundancy) {
+        validate(fixture, redundancy, false);
+    }
+
+    private static void validate(Fixture fixture, Redundancy redundancy, boolean enableMultipleBucketSpaces) {
         new GlobalDistributionValidator()
-                .validate(fixture.getDocumentTypes(), fixture.getGloballyDistributedDocuments());
+                .validate(fixture.getDocumentTypes(), fixture.getGloballyDistributedDocuments(), redundancy, enableMultipleBucketSpaces);
     }
 
     private static class Fixture {
diff --git a/storage/src/vespa/storage/storageserver/storagenode.cpp b/storage/src/vespa/storage/storageserver/storagenode.cpp
index 6bb2ca31ec1..ad98d64b173 100644
--- a/storage/src/vespa/storage/storageserver/storagenode.cpp
+++ b/storage/src/vespa/storage/storageserver/storagenode.cpp
@@ -206,8 +206,10 @@ StorageNode::initialize()
 
     _chain.reset(createChain().release());
 
-    assert(_communicationManager != nullptr);
-    _communicationManager->updateBucketSpacesConfig(*_bucketSpacesConfig);
+    if (_component->enableMultipleBucketSpaces()) {
+        assert(_communicationManager != nullptr);
+        _communicationManager->updateBucketSpacesConfig(*_bucketSpacesConfig);
+    }
 
     // Start the metric manager, such that it starts generating snapshots
     // and the like. Note that at this time, all metrics should hopefully
@@ -357,7 +359,9 @@ StorageNode::handleLiveConfigUpdate(const InitialGuard & initGuard)
     if (_newBucketSpacesConfig) {
         _bucketSpacesConfig = std::move(_newBucketSpacesConfig);
         _context.getComponentRegister().setBucketSpacesConfig(*_bucketSpacesConfig);
-        _communicationManager->updateBucketSpacesConfig(*_bucketSpacesConfig);
+        if (_component->enableMultipleBucketSpaces()) {
+            _communicationManager->updateBucketSpacesConfig(*_bucketSpacesConfig);
+        }
     }
 }
 
diff --git a/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.cpp b/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.cpp
index f83188f7dd8..33fca0f161a 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.cpp
+++ b/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.cpp
@@ -17,12 +17,12 @@ mbus::string StorageProtocol::NAME = "StorageProtocol";
 
 StorageProtocol::StorageProtocol(const std::shared_ptr<const document::DocumentTypeRepo> repo,
                                  const documentapi::LoadTypeSet& loadTypes,
-                                 bool configForcedBucketSpaceSerialization)
+                                 bool activateBucketSpaceSerialization)
     : _serializer5_0(repo, loadTypes),
       _serializer5_1(repo, loadTypes),
       _serializer5_2(repo, loadTypes),
       _serializer6_0(repo, loadTypes),
-      _configForcedBucketSpaceSerialization(configForcedBucketSpaceSerialization)
+      _activateBucketSpaceSerialization(activateBucketSpaceSerialization)
 {
 }
 
@@ -106,7 +106,7 @@ StorageProtocol::encode(const vespalib::Version& version,
         } else if (version < version5_2) {
             return encodeMessage(_serializer5_1, routable, message, version5_1, version);
         } else {
-            if (_configForcedBucketSpaceSerialization) {
+            if (_activateBucketSpaceSerialization) {
                 return encodeMessage(_serializer6_0, routable, message, version6_0, version);
             } else {
                if (version < version6_0) {
@@ -184,7 +184,7 @@ StorageProtocol::decode(const vespalib::Version & version,
         } else if (version < version5_2) {
             return decodeMessage(_serializer5_1, data, type, version5_1, version);
         } else {
-            if (_configForcedBucketSpaceSerialization) {
+            if (_activateBucketSpaceSerialization) {
                 return decodeMessage(_serializer6_0, data, type, version6_0, version);
             } else {
                 if (version < version6_0) {
diff --git a/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.h b/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.h
index 56f271db1d0..d85e9d55d1a 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.h
+++ b/storageapi/src/vespa/storageapi/mbusprot/storageprotocol.h
@@ -29,7 +29,7 @@ private:
     ProtocolSerialization5_1 _serializer5_1;
     ProtocolSerialization5_2 _serializer5_2;
     ProtocolSerialization6_0 _serializer6_0;
-    bool _configForcedBucketSpaceSerialization;
+    bool _activateBucketSpaceSerialization;
 };
 
 }
diff --git a/storageserver/src/tests/testhelper.cpp b/storageserver/src/tests/testhelper.cpp
index 5e6a71b078f..b245a6500bd 100644
--- a/storageserver/src/tests/testhelper.cpp
+++ b/storageserver/src/tests/testhelper.cpp
@@ -96,11 +96,7 @@ vdstestlib::DirConfig getStandardConfig(bool storagenode) {
     // By default, need "old" behaviour of maxconcurrent
     config->set("maxconcurrentvisitors_fixed", "4");
     config->set("maxconcurrentvisitors_variable", "0");
-    dc.addConfig("stor-visitordispatcher");
-    config = &dc.addConfig("bucketspaces");
-    config->set("documenttype[1]");
-    config->set("documenttype[0].name", "testdoctype1");
-    config->set("documenttype[0].bucketspace", "default");
+    config = &dc.addConfig("stor-visitordispatcher");
     addFileConfig(dc, "documenttypes", "config-doctypes.cfg");
     addStorageDistributionConfig(dc);
     return dc;