Merge pull request #23444 from vespa-engine/hmusum/deprecate-docproc-attributes

Add deploy warnings for deprecated and ignored docproc config

6 files changed, 70 insertions, 132 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/docproc/ContainerDocproc.java b/config-model/src/main/java/com/yahoo/vespa/model/container/docproc/ContainerDocproc.java
index fee10b965aa..242f7cbefdb 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/docproc/ContainerDocproc.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/docproc/ContainerDocproc.java
@@ -15,35 +15,26 @@ import java.util.HashMap;
 import java.util.Map;
 
 /**
- * @author einarmr
+ * @author Einar M R Rosenvinge
  * @author gjoranv
  */
-public class ContainerDocproc extends ContainerSubsystem<DocprocChains>
-        implements 
-            ContainerMbusConfig.Producer,
-            SchemamappingConfig.Producer,
-            DocprocConfig.Producer
-{
-    public final Options options;
-
-    // Whether or not to prefer sending to a local node.
-    private final boolean preferLocalNode = false;
-
-    // The number of nodes to use per client.
-    private final int numNodesPerClient = 0;
+public class ContainerDocproc extends ContainerSubsystem<DocprocChains> implements
+        ContainerMbusConfig.Producer,
+        SchemamappingConfig.Producer,
+        DocprocConfig.Producer {
 
+    public final Options options;
     private final Map<Pair<String, String>, String> fieldNameSchemaMap = new HashMap<>();
 
     public ContainerDocproc(ContainerCluster<?> cluster, DocprocChains chains) {
-        this(cluster, chains, new Options( null, null, null, null, null, null));
+        this(cluster, chains, Options.empty());
     }
 
     public ContainerDocproc(ContainerCluster<?> cluster, DocprocChains chains, Options options) {
         this(cluster, chains, options, true);
     }
 
-    private void addSource(
-            final ContainerCluster<?> cluster, final String name, final SessionConfig.Type.Enum type) {
+    private void addSource(ContainerCluster<?> cluster, String name, SessionConfig.Type.Enum type) {
         final MbusClient mbusClient = new MbusClient(name, type);
         mbusClient.addClientBindings(SystemBindingPattern.fromPattern("mbus://*/" + mbusClient.getSessionName()));
         cluster.addComponent(mbusClient);
@@ -61,14 +52,6 @@ public class ContainerDocproc extends ContainerSubsystem<DocprocChains>
         cluster.addSearchAndDocprocBundles();
     }
 
-    public boolean isPreferLocalNode() {
-        return preferLocalNode;
-    }
-
-    public int getNumNodesPerClient() {
-        return numNodesPerClient;
-    }
-
     @Override
     public void getConfig(ContainerMbusConfig.Builder builder) {
         builder.maxpendingcount(getMaxMessagesInQueue());
@@ -79,13 +62,9 @@ public class ContainerDocproc extends ContainerSubsystem<DocprocChains>
             return options.maxMessagesInQueue;
         }
 
-        //maxmessagesinqueue has not been set for this node. let's try to give a good value anyway:
+        // maxmessagesinqueue has not been set for this node. let's try to give a good value anyway:
         return 2048 * getChains().allChains().allComponents().size();
-        //intentionally high, getMaxQueueMbSize() will probably kick in before this one!
-    }
-
-    private Integer getMaxQueueMbSize() {
-        return options.maxQueueMbSize;
+        // intentionally high, getMaxQueueMbSize() will probably kick in before this one!
     }
 
     private Integer getMaxQueueTimeMs() {
@@ -134,21 +113,22 @@ public class ContainerDocproc extends ContainerSubsystem<DocprocChains>
     public static class Options {
 
         public final Integer maxMessagesInQueue;
-        public final Integer maxQueueMbSize;
         public final Integer maxQueueTimeMs;
 
         public final Double maxConcurrentFactor;
         public final Double documentExpansionFactor;
         public final Integer containerCoreMemory;
 
-        public Options(Integer maxMessagesInQueue, Integer maxQueueMbSize, Integer maxQueueTimeMs, Double maxConcurrentFactor, Double documentExpansionFactor, Integer containerCoreMemory) {
+        public Options(Integer maxMessagesInQueue, Integer maxQueueTimeMs, Double maxConcurrentFactor, Double documentExpansionFactor, Integer containerCoreMemory) {
             this.maxMessagesInQueue = maxMessagesInQueue;
-            this.maxQueueMbSize = maxQueueMbSize;
             this.maxQueueTimeMs = maxQueueTimeMs;
             this.maxConcurrentFactor = maxConcurrentFactor;
             this.documentExpansionFactor = documentExpansionFactor;
             this.containerCoreMemory = containerCoreMemory;
         }
+
+        static Options empty() { return new Options(null, null, null, null, null); }
+
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index a8ce0219434..8f02476b2cc 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -943,7 +943,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         addIncludes(docprocElement);
         DocprocChains chains = new DomDocprocChainsBuilder(null, false).build(deployState, cluster, docprocElement);
 
-        ContainerDocproc.Options docprocOptions = DocprocOptionsBuilder.build(docprocElement);
+        ContainerDocproc.Options docprocOptions = DocprocOptionsBuilder.build(docprocElement, deployState.getDeployLogger());
         return new ContainerDocproc(cluster, chains, docprocOptions, !standaloneBuilder);
      }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/DocprocOptionsBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/DocprocOptionsBuilder.java
index e0dffca2bc5..faf2d01d385 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/DocprocOptionsBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/DocprocOptionsBuilder.java
@@ -1,17 +1,18 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.container.xml;
 
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.vespa.model.container.docproc.ContainerDocproc;
 import org.w3c.dom.Element;
+import java.util.Set;
+import java.util.logging.Level;
 
-/**
- * Extracted from DomDocProcClusterBuilder
- */
 public class DocprocOptionsBuilder {
-    public static ContainerDocproc.Options build(Element spec) {
+    public static ContainerDocproc.Options build(Element spec, DeployLogger deployLogger) {
+        checkForDeprecatedAttributes(spec, Set.of("maxqueuebytesize", "numnodesperclient", "preferlocalnode"), deployLogger);
+
         return new ContainerDocproc.Options(
                 getMaxMessagesInQueue(spec),
-                getSizeInMegabytes(spec.getAttribute("maxqueuebytesize")),
                 getTime(spec.getAttribute("maxqueuewait")),
                 getFactor(spec.getAttribute("maxconcurrentfactor")),
                 getFactor(spec.getAttribute("documentexpansionfactor")),
@@ -19,18 +20,17 @@ public class DocprocOptionsBuilder {
     }
 
     private static Integer getInt(String integer) {
-        return integer == null || integer.trim().isEmpty() ?
-                null:
-                Integer.parseInt(integer);
+        return integer == null || integer.trim().isEmpty()
+                ? null
+                : Integer.parseInt(integer);
     }
 
     private static Double getFactor(String factor) {
-        return factor == null || factor.trim().isEmpty() ?
-                null :
-                Double.parseDouble(factor);
+        return factor == null || factor.trim().isEmpty()
+                ? null
+                : Double.parseDouble(factor);
     }
 
-
     private static Integer getMaxMessagesInQueue(Element spec) {
         // get max queue size (number of messages), if set
         Integer maxMessagesInQueue = null;
@@ -40,28 +40,6 @@ public class DocprocOptionsBuilder {
         return maxMessagesInQueue;
     }
 
-    private static Integer getSizeInMegabytes(String size) {
-        if (size == null) {
-            return null;
-        }
-        size = size.trim();
-        if (size.isEmpty()) {
-            return null;
-        }
-
-        Integer megabyteSize;
-        if (size.endsWith("m")) {
-            size = size.substring(0, size.length() - 1);
-            megabyteSize = Integer.parseInt(size);
-        } else if (size.endsWith("g")) {
-            size = size.substring(0, size.length() - 1);
-            megabyteSize = Integer.parseInt(size) * 1024;
-        } else {
-            throw new IllegalArgumentException("Heap sizes for docproc must be set to Xm or Xg, where X is an integer specifying megabytes or gigabytes, respectively.");
-        }
-        return megabyteSize;
-    }
-
     private static Integer getTime(String intStr) {
         if (intStr == null) {
             return null;
@@ -73,4 +51,13 @@ public class DocprocOptionsBuilder {
 
         return 1000 * (int)Double.parseDouble(intStr);
     }
+
+    private static void checkForDeprecatedAttributes(Element spec, Set<String> names, DeployLogger deployLogger) {
+        names.forEach(n -> {
+            if (!spec.getAttribute(n).isEmpty())
+                deployLogger.logApplicationPackage(Level.WARNING, "'" + n + "' is ignored, deprecated and will be removed in Vespa 9.");
+        });
+    }
+
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/routing/DocumentProtocol.java b/config-model/src/main/java/com/yahoo/vespa/model/routing/DocumentProtocol.java
index 754800a42e1..8a8d38e23e3 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/routing/DocumentProtocol.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/routing/DocumentProtocol.java
@@ -182,44 +182,26 @@ public final class DocumentProtocol implements Protocol,
         return table;
     }
 
-    private static void addContainerClusterDocprocHops(Collection<ContainerCluster<?>> containerClusters,
-                                                       RoutingTableSpec table) {
-
+    private static void addContainerClusterDocprocHops(Collection<ContainerCluster<?>> containerClusters, RoutingTableSpec table) {
         for (ContainerCluster<?> cluster: containerClusters) {
             ContainerDocproc docproc = cluster.getDocproc();
 
             if (docproc != null) {
-                String policy = policy(docproc);
-
                 for (DocprocChain chain : docproc.getChains().allChains().allComponents()) {
-                    addChainHop(table, cluster.getConfigId(), policy, chain);
+                    addChainHop(table, cluster.getConfigId(), chain);
                 }
             }
         }
     }
 
-    private static void addChainHop(RoutingTableSpec table, String configId, String policy, DocprocChain chain) {
-        final StringBuilder selector = new StringBuilder();
-        if (policy != null) {
-            selector.append(configId).append("/").append(policy).append("/").append(chain.getSessionName());
-        } else {
-            selector.append("[LoadBalancer:cluster=").append(configId)
-                    .append(";session=").append(chain.getSessionName())
-                    .append("]");
-        }
+    private static void addChainHop(RoutingTableSpec table, String configId, DocprocChain chain) {
+        StringBuilder selector = new StringBuilder();
+        selector.append("[LoadBalancer:cluster=").append(configId)
+                .append(";session=").append(chain.getSessionName())
+                .append("]");
         table.addHop(new HopSpec(chain.getServiceName(), selector.toString()));
     }
 
-    private static String policy(ContainerDocproc docproc) {
-        if (docproc.getNumNodesPerClient() > 0) {
-            return "[SubsetService:" + docproc.getNumNodesPerClient() + "]";
-        } else if (docproc.isPreferLocalNode()) {
-            return "[LocalService]";
-        } else {
-            return null;
-        }
-    }
-
     /**
      * Create hops to all configured storage nodes for the Document protocol. The "Distributor" policy resolves its
      * recipients using slobrok lookups, so it requires no configured recipients.
@@ -228,23 +210,22 @@ public final class DocumentProtocol implements Protocol,
      * @param table   the routing table to add to
      */
     private static void addContentRouting(List<ContentCluster> content, RoutingTableSpec table) {
-
-            for (ContentCluster cluster : content) {
-                RouteSpec spec = new RouteSpec(cluster.getConfigId());
-
-                if (cluster.getSearch().hasIndexedCluster()) {
-                    table.addRoute(spec.addHop("[MessageType:" + cluster.getConfigId() + "]"));
-                    table.addRoute(new RouteSpec(getIndexedRouteName(cluster.getConfigId()))
-                                           .addHop(cluster.getSearch().getIndexed().getIndexingServiceName())
-                                           .addHop("[Content:cluster=" + cluster.getName() + "]"));
-                    table.addRoute(new RouteSpec(getDirectRouteName(cluster.getConfigId()))
-                                           .addHop("[Content:cluster=" + cluster.getName() + "]"));
-                } else {
-                    table.addRoute(spec.addHop("[Content:cluster=" + cluster.getName() + "]"));
-                }
-                table.addRoute(new RouteSpec("storage/cluster." + cluster.getName())
-                                       .addHop("route:" + cluster.getConfigId()));
+        for (ContentCluster cluster : content) {
+            RouteSpec spec = new RouteSpec(cluster.getConfigId());
+
+            if (cluster.getSearch().hasIndexedCluster()) {
+                table.addRoute(spec.addHop("[MessageType:" + cluster.getConfigId() + "]"));
+                table.addRoute(new RouteSpec(getIndexedRouteName(cluster.getConfigId()))
+                                       .addHop(cluster.getSearch().getIndexed().getIndexingServiceName())
+                                       .addHop("[Content:cluster=" + cluster.getName() + "]"));
+                table.addRoute(new RouteSpec(getDirectRouteName(cluster.getConfigId()))
+                                       .addHop("[Content:cluster=" + cluster.getName() + "]"));
+            } else {
+                table.addRoute(spec.addHop("[Content:cluster=" + cluster.getName() + "]"));
             }
+            table.addRoute(new RouteSpec("storage/cluster." + cluster.getName())
+                                   .addHop("route:" + cluster.getConfigId()));
+        }
     }
 
     /**
@@ -255,9 +236,8 @@ public final class DocumentProtocol implements Protocol,
      * @param table the routing table to add to
      */
     private static void addIndexingHop(List<ContentCluster> content, RoutingTableSpec table) {
-        if (content.isEmpty()) {
-            return;
-        }
+        if (content.isEmpty()) return;
+
         HopSpec hop = new HopSpec("indexing", "[DocumentRouteSelector]");
         for (ContentCluster cluster : content) {
             hop.addRecipient(cluster.getConfigId());
@@ -281,9 +261,8 @@ public final class DocumentProtocol implements Protocol,
     private static void addDefaultRoutes(List<ContentCluster> content,
                                          Collection<ContainerCluster<?>> containerClusters,
                                          RoutingTableSpec table) {
-        if (content.isEmpty() || !indexingHopExists(table)) {
-            return;
-        }
+        if (content.isEmpty() || !indexingHopExists(table)) return;
+
         RouteSpec route = new RouteSpec("default");
         String hop = getContainerClustersDocprocHop(containerClusters);
         if (hop != null) {
@@ -326,9 +305,9 @@ public final class DocumentProtocol implements Protocol,
     }
 
     private static DocprocChain getDefaultChain(ContainerDocproc docproc) {
-        return docproc == null ?
-                null:
-                docproc.getChains().allChains().getComponent("default");
+        return docproc == null
+                ? null
+                : docproc.getChains().allChains().getComponent("default");
     }
 
     /**
diff --git a/config-model/src/main/resources/schema/docproc.rnc b/config-model/src/main/resources/schema/docproc.rnc
index 42902f7180f..11f8e14fb2d 100644
--- a/config-model/src/main/resources/schema/docproc.rnc
+++ b/config-model/src/main/resources/schema/docproc.rnc
@@ -11,9 +11,6 @@ SchemaMapping = element map {
   }+
 }
 
-
-
-
 #Version 3 config:
 
 DocProcV3 = attribute version { "3.0" },
@@ -24,10 +21,10 @@ DocProcV3 = attribute version { "3.0" },
 
 # TODO Here we need a thorough cleaning
 DocprocClusterAttributes =   attribute compressdocuments { xsd:boolean }? &
-  attribute numnodesperclient { xsd:positiveInteger }? &
-  attribute preferlocalnode { xsd:boolean }? &
+  attribute numnodesperclient { xsd:positiveInteger }? & # TODO: Remove in Vespa 9
+  attribute preferlocalnode { xsd:boolean }? & # TODO: Remove in Vespa 9
   attribute maxmessagesinqueue { xsd:nonNegativeInteger }? &
-  attribute maxqueuebytesize { xsd:string { minLength = "1" } }? &
+  attribute maxqueuebytesize { xsd:string { minLength = "1" } }? &  # TODO: Remove in Vespa 9
   attribute maxqueuewait { xsd:positiveInteger }? &
   attribute maxconcurrentfactor { xsd:double { minExclusive = "0.0" maxExclusive = "1.0" } }? &
   attribute documentexpansionfactor { xsd:double { minExclusive = "0.0" } }? &
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/DocprocBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/DocprocBuilderTest.java
index 2044fd2ab39..eee421e8037 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/DocprocBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/DocprocBuilderTest.java
@@ -27,11 +27,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
-
 /**
- * @author einarmr
+ * @author Einar M R Rosenvinge
  * @author gjoranv
- * @since 5.1.9
  */
 public class DocprocBuilderTest extends DomBuilderTest {
 
@@ -64,7 +62,7 @@ public class DocprocBuilderTest extends DomBuilderTest {
                 "  <nodes>",
                 "    <node hostalias='mockhost' baseport='1500' />",
                 "  </nodes>",
-                "  <document-processing preferlocalnode='true' numnodesperclient='2' maxqueuebytesize='100m' maxmessagesinqueue='300' maxqueuewait='200'>",
+                "  <document-processing maxmessagesinqueue='300' maxqueuewait='200'>",
                 "    <documentprocessor id='docproc1' class='com.yahoo.Docproc1' bundle='docproc1bundle'/>",
                 "    <chain id='chein'>",
                 "      <documentprocessor id='docproc2'/>",
@@ -73,12 +71,9 @@ public class DocprocBuilderTest extends DomBuilderTest {
                 "</container>");
     }
 
-    // TODO: re-enable assertions when the appropriate attributes are handled by the builder
     @Test
     public void testDocprocCluster() {
         assertEquals("banan", cluster.getName());
-        //assertTrue(cluster.getContainerDocproc().isPreferLocalNode());
-        //assertEquals(2, cluster.getContainerDocproc().getNumNodesPerClient());
         List<ApplicationContainer> services = cluster.getContainers();
         assertEquals(1, services.size());
         ApplicationContainer service = services.get(0);