Log a deployment warning if distribution keys are high relative to node count

Using very high distribution keys will non-obviously slow down bucket
operations in the system, including feed. Log a warning if such a
case is detected, including a link to the documentation.

2 files changed, 84 insertions, 1 deletions

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 bac86e37e8f..ae890408b99 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
@@ -46,6 +46,7 @@ import com.yahoo.vespa.model.content.IndexedHierarchicDistributionValidator;
 import com.yahoo.vespa.model.content.Redundancy;
 import com.yahoo.vespa.model.content.ReservedDocumentTypeNameValidator;
 import com.yahoo.vespa.model.content.StorageGroup;
+import com.yahoo.vespa.model.content.StorageNode;
 import com.yahoo.vespa.model.content.engines.PersistenceEngine;
 import com.yahoo.vespa.model.content.engines.ProtonEngine;
 import com.yahoo.vespa.model.content.storagecluster.StorageCluster;
@@ -137,6 +138,7 @@ public class ContentCluster extends TreeConfigProducer<AnyConfigProducer> implem
             c.rootGroup = new StorageGroup.Builder(contentElement, context).buildRootGroup(deployState, c, c.search.isStreaming());
             c.clusterControllerConfig = createClusterControllerConfig(contentElement, deployState, c, resourceLimits);
             validateThatGroupSiblingsAreUnique(c.clusterId, c.rootGroup);
+            warnIfDistributionKeyRangeIsSuboptimal(c.clusterId, c.rootGroup, deployState);
             c.search.handleRedundancy(c.redundancy);
             setupSearchCluster(c.search, contentElement, deployState.getDeployLogger());
 
@@ -275,6 +277,38 @@ public class ContentCluster extends TreeConfigProducer<AnyConfigProducer> implem
             }
         }
 
+        private static class HighestDistributionKeyAggregator {
+            public int nodeCount = 0;
+            public int highestNodeDistributionKey = 0;
+
+            void aggregateNodeStats(StorageGroup group) {
+                for (StorageNode n : group.getNodes()) {
+                    nodeCount++;
+                    highestNodeDistributionKey = Math.max(highestNodeDistributionKey, n.getDistributionKey());
+                }
+                for (StorageGroup g : group.getSubgroups()) {
+                    aggregateNodeStats(g);
+                }
+            }
+        }
+
+        private void warnIfDistributionKeyRangeIsSuboptimal(String clusterId, StorageGroup rootGroup, DeployState deployState) {
+            if (rootGroup == null) {
+                return; // Unit testing case
+            }
+            var aggr = new HighestDistributionKeyAggregator();
+            aggr.aggregateNodeStats(rootGroup);
+            int warnThreshold = 100; // ... Not scientifically chosen
+            if ((aggr.highestNodeDistributionKey - aggr.nodeCount) >= warnThreshold) {
+                deployState.getDeployLogger().logApplicationPackage(WARNING,
+                        ("Content cluster '%s' has %d node(s), but the highest distribution key is %d. " +
+                         "Having much higher distribution keys than the number of nodes is not recommended, " +
+                         "as it may negatively affect performance. " +
+                         "See https://docs.vespa.ai/en/reference/services-content.html#node")
+                        .formatted(clusterId, aggr.nodeCount, aggr.highestNodeDistributionKey));
+            }
+        }
+
         private void addClusterControllers(ConfigModelContext context,
                                            ModelElement contentElement,
                                            ContentCluster contentCluster,
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/ContentClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/ContentClusterTest.java
index a24abc21b76..5dca5a9eaa1 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/ContentClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/ContentClusterTest.java
@@ -1,6 +1,7 @@
 // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.content;
 
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ApplicationClusterEndpoint;
 import com.yahoo.config.model.api.ContainerEndpoint;
 import com.yahoo.config.model.api.ModelContext;
@@ -49,6 +50,7 @@ import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.logging.Level;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -942,8 +944,19 @@ public class ContentClusterTest extends ContentBaseTest {
     }
 
     private static ContentCluster createOneNodeCluster(String clusterXml, TestProperties props, Optional<Flavor> flavor) throws Exception {
+        return createOneNodeCluster(clusterXml, props, flavor, new StringBuffer());
+    }
+
+    private static ContentCluster createOneNodeCluster(String clusterXml, TestProperties props,
+                                                       Optional<Flavor> flavor, StringBuffer deployWarningsBuffer) throws Exception {
+        DeployLogger logger = (level, message) -> {
+            if (level == Level.WARNING) { // only care about warnings
+                deployWarningsBuffer.append("%s\n".formatted(message));
+            }
+        };
         DeployState.Builder deployStateBuilder = new DeployState.Builder()
-                .properties(props);
+                .properties(props)
+                .deployLogger(logger);
         MockRoot root = flavor.isPresent() ?
                 ContentClusterUtils.createMockRoot(new SingleNodeProvisioner(flavor.get()),
                         List.of(), deployStateBuilder) :
@@ -1527,6 +1540,42 @@ public class ContentClusterTest extends ContentBaseTest {
         }
     }
 
+    private String createClusterAndGetDeploymentWarnings(String xml) {
+        var warningBuf = new StringBuffer();
+        try {
+            createOneNodeCluster(xml, new TestProperties(), Optional.empty(), warningBuf);
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+        return warningBuf.toString();
+    };
+
+    private static String clusterXmlWithNodeDistributionKey(int key) {
+        return "<content version=\"1.0\" id=\"mockcluster\">" +
+               "  <redundancy>1</redundancy>" +
+               "  <documents/>" +
+               "  <group>" +
+               "    <node distribution-key=\"%d\" hostalias=\"mockhost\"/>".formatted(key) +
+               "  </group>" +
+               "</content>";
+    }
+
+    @Test
+    void distribution_key_much_higher_than_node_count_logs_deployment_warning() {
+        // "much higher" is somewhat arbitrary, but needs to be kept in track with threshold in `ContentCluster`.
+        String warnings = createClusterAndGetDeploymentWarnings(clusterXmlWithNodeDistributionKey(101));
+        assertEquals(warnings, "Content cluster 'mockcluster' has 1 node(s), but the highest distribution " +
+                               "key is 101. Having much higher distribution keys than the number of nodes " +
+                               "is not recommended, as it may negatively affect performance. " +
+                               "See https://docs.vespa.ai/en/reference/services-content.html#node\n");
+    }
+
+    @Test
+    void distribution_key_not_much_higher_than_node_count_does_not_log_deployment_warning() {
+        String warnings = createClusterAndGetDeploymentWarnings(clusterXmlWithNodeDistributionKey(100));
+        assertEquals(warnings, "");
+    }
+
     private String servicesWithGroups(int groupCount, double minGroupUpRatio) {
         String services = String.format("<?xml version='1.0' encoding='UTF-8' ?>" +
                 "<services version='1.0'>" +