Revert "Revert "Bratseth/only one clustercontroller per host 2""

This reverts commit f9b197a293c63e824cf1062b5642b67a3f00c16e.

7 files changed, 95 insertions, 26 deletions

diff --git a/application/src/main/java/com/yahoo/application/container/Processing.java b/application/src/main/java/com/yahoo/application/container/Processing.java
index 1511024364d..085067f707c 100644
--- a/application/src/main/java/com/yahoo/application/container/Processing.java
+++ b/application/src/main/java/com/yahoo/application/container/Processing.java
@@ -22,6 +22,7 @@ import java.io.IOException;
 */
 @Beta
 public final class Processing extends ProcessingBase<Request, Response, Processor> {
+
     private final ProcessingHandler handler;
 
     Processing(ProcessingHandler handler) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
index 13d85c3a955..187a6222648 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
@@ -114,7 +114,7 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon
         }
         if (hostResource == null) {
             throw new RuntimeException("No host found for service '" + getServiceName() + "'. " +
-                    "The hostalias is probably missing from hosts.xml.");
+                                       "The hostalias is probably missing from hosts.xml.");
         }
         id = getIndex(hostResource);
         ports = hostResource.allocateService(this, getInstanceWantedPort(userPort));
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerCluster.java
index b82f58fbb40..3847c32dfb2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerCluster.java
@@ -43,7 +43,7 @@ public class ClusterControllerCluster extends AbstractConfigProducer<ContainerCl
 
     @Override
     public void getConfig(ZookeepersConfig.Builder builder) {
-        final Collection<String> controllerHosts = new ArrayList<>();
+        Collection<String> controllerHosts = new ArrayList<>();
         for (Container container : containerCluster.getContainers()) {
             controllerHosts.add(container.getHostName() + ":" + ZK_CLIENT_PORT);
         }
@@ -73,5 +73,6 @@ public class ClusterControllerCluster extends AbstractConfigProducer<ContainerCl
             }
         }
     }
+
 }
 
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 8350ac3fc79..669320165cc 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
@@ -5,6 +5,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.Sets;
 import com.yahoo.config.model.ConfigModelUtils;
 import com.yahoo.config.model.producer.AbstractConfigProducerRoot;
+import com.yahoo.config.model.provision.Host;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.vespa.config.content.MessagetyperouteselectorpolicyConfig;
@@ -16,7 +17,9 @@ import com.yahoo.documentapi.messagebus.protocol.DocumentProtocol;
 import com.yahoo.metrics.MetricsmanagerConfig;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.vespa.model.HostResource;
+import com.yahoo.vespa.model.Service;
 import com.yahoo.vespa.model.admin.Admin;
+import com.yahoo.vespa.model.admin.Configserver;
 import com.yahoo.vespa.model.admin.Metric;
 import com.yahoo.vespa.model.admin.MetricsConsumer;
 import com.yahoo.vespa.model.admin.MonitoringSystem;
@@ -319,8 +322,8 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri
 
         private List<HostResource> drawControllerHosts(int count, StorageGroup rootGroup, Collection<ContainerModel> containers) {
             List<HostResource> hosts = drawContentHostsRecursively(count, rootGroup);
-            //if (hosts.size() < count) // supply with containers TODO: Reactivate
-            //    hosts.addAll(drawContainerHosts(count - hosts.size(), containers));
+            if (hosts.size() < count) // supply with containers
+                hosts.addAll(drawContainerHosts(count - hosts.size(), containers));
             if (hosts.size() % 2 == 0) // ZK clusters of even sizes are less available (even in the size=2 case)
                 hosts = hosts.subList(0, hosts.size()-1);
             return hosts;
@@ -336,10 +339,15 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri
         private List<HostResource> drawContainerHosts(int count, Collection<ContainerModel> containerClusters) {
             if (containerClusters.isEmpty()) return Collections.emptyList();
 
-            List<HostResource> hosts = new ArrayList<>();
+            List<HostResource> allHosts = new ArrayList<>();
             for (ContainerCluster cluster : clustersSortedByName(containerClusters))
-                hosts.addAll(hostResourcesSortedByIndex(cluster));
-            return hosts.subList(0, Math.min(hosts.size(), count));
+                allHosts.addAll(hostResourcesSortedByIndex(cluster));
+
+            // Don't use the same container to supplement multiple content clusters
+            List<HostResource> hostsWithoutClusterController =
+                    allHosts.stream().filter(h -> ! hostHasClusterController(h.getHostName(), allHosts)).collect(Collectors.toList());
+
+            return hostsWithoutClusterController.subList(0, Math.min(hostsWithoutClusterController.size(), count));
         }
         
         private List<ContainerCluster> clustersSortedByName(Collection<ContainerModel> containerModels) {
@@ -356,6 +364,23 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri
                     .collect(Collectors.toList());
         }
 
+        /** Returns whether any host having the given hostname has a cluster controller */
+        private boolean hostHasClusterController(String hostname, List<HostResource> allHosts) {
+            for (HostResource host : allHosts) {
+                if ( ! host.getHostName().equals(hostname)) continue;
+                if (! hasClusterController(host)) continue;
+                return true;
+            }
+            return false;
+        }
+
+        private boolean hasClusterController(HostResource host) {
+            for (Service service : host.getServices())
+                if (service instanceof ClusterControllerContainer)
+                    return true;
+            return false;
+        }
+
         /**
          * Draw <code>count</code> nodes from as many different content groups below this as possible.
          * This will only achieve maximum spread in the case where the groups are balanced and never on the same
diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
index e4f405efb93..1c7940fdc8f 100644
--- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
+++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
@@ -458,7 +458,7 @@ public class ModelProvisioningTest {
 
         ContentCluster cluster = model.getContentClusters().get("bar");
         ContainerCluster clusterControllers = cluster.getClusterControllers();
-        assertEquals(1, clusterControllers.getContainers().size()); // TODO: Expected 5 with this feature reactivated
+        assertEquals(5, clusterControllers.getContainers().size());
     }
 
     public void testClusterControllersAreNotPlacedOnRetiredNodes() {
@@ -619,17 +619,63 @@ public class ModelProvisioningTest {
                 "  </content>" +
                 "</services>";
 
-        int numberOfHosts = 5;
         VespaModelTester tester = new VespaModelTester();
-        tester.addHosts(numberOfHosts);
+        tester.addHosts(5);
         VespaModel model = tester.createModel(services, true);
-        assertThat(model.getRoot().getHostSystem().getHosts().size(), is(numberOfHosts));
 
         ContentCluster cluster = model.getContentClusters().get("bar");
         ContainerCluster clusterControllers = cluster.getClusterControllers();
-        assertEquals(1, clusterControllers.getContainers().size()); // TODO: Expected 3 with this feature reactivated
+        assertEquals(3, clusterControllers.getContainers().size());
     }
 
+    @Test
+    public void test2ContentNodesOn2ClustersWithContainerClusterProducesMixedClusterControllerCluster() throws ParseException {
+        String services =
+                "<?xml version='1.0' encoding='utf-8' ?>\n" +
+                "<services>" +
+                "  <container version='1.0' id='container'>" +
+                "     <nodes count='3' flavor='container-node'/>" +
+                "  </container>" +
+                "  <content version='1.0' id='content1'>" +
+                "     <redundancy>2</redundancy>" +
+                "     <documents>" +
+                "       <document type='type1' mode='index'/>" +
+                "     </documents>" +
+                "     <nodes count='2' flavor='content1-node'/>" +
+                "  </content>" +
+                "  <content version='1.0' id='content2'>" +
+                "     <redundancy>2</redundancy>" +
+                "     <documents>" +
+                "       <document type='type1' mode='index'/>" +
+                "     </documents>" +
+                "     <nodes count='2' flavor='content2-node'/>" +
+                "  </content>" +
+                "</services>";
+
+        VespaModelTester tester = new VespaModelTester();
+        // use different flavors to make the test clearer
+        tester.addHosts("container-node", 3);
+        tester.addHosts("content1-node",  2);
+        tester.addHosts("content2-node",  2);
+        VespaModel model = tester.createModel(services, true);
+
+        ContentCluster cluster1 = model.getContentClusters().get("content1");
+        ContainerCluster clusterControllers1 = cluster1.getClusterControllers();
+        assertEquals(3, clusterControllers1.getContainers().size());
+        assertEquals("content1-node0",  clusterControllers1.getContainers().get(0).getHostName());
+        assertEquals("content1-node1",  clusterControllers1.getContainers().get(1).getHostName());
+        assertEquals("container-node0", clusterControllers1.getContainers().get(2).getHostName());
+
+        ContentCluster cluster2 = model.getContentClusters().get("content2");
+        ContainerCluster clusterControllers2 = cluster2.getClusterControllers();
+        assertEquals(3, clusterControllers2.getContainers().size());
+        assertEquals("content2-node0",  clusterControllers2.getContainers().get(0).getHostName());
+        assertEquals("content2-node1",  clusterControllers2.getContainers().get(1).getHostName());
+        assertEquals("We do not pick the container used to supplement another cluster",
+                     "container-node1", clusterControllers2.getContainers().get(2).getHostName());
+    }
+
+    @Test
     public void testExplicitDedicatedClusterControllers() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
diff --git a/container-core/src/main/java/com/yahoo/processing/handler/ProcessingResponse.java b/container-core/src/main/java/com/yahoo/processing/handler/ProcessingResponse.java
index 2ce345ad355..d9bbd30a474 100644
--- a/container-core/src/main/java/com/yahoo/processing/handler/ProcessingResponse.java
+++ b/container-core/src/main/java/com/yahoo/processing/handler/ProcessingResponse.java
@@ -38,7 +38,6 @@ public class ProcessingResponse extends AsyncHttpResponse {
 
     private final com.yahoo.processing.Request processingRequest;
     private final com.yahoo.processing.Response processingResponse;
-    private final Executor renderingExecutor;
     private final Execution execution;
     private final Renderer renderer;
 
@@ -46,24 +45,21 @@ public class ProcessingResponse extends AsyncHttpResponse {
     private boolean explicitStatusSet = false;
 
     @SuppressWarnings("unchecked")
-    public ProcessingResponse(
-            int status,
-            final com.yahoo.processing.Request processingRequest,
-            final com.yahoo.processing.Response processingResponse,
-            final Renderer renderer,
-            final Executor renderingExecutor, final Execution execution) {
+    public ProcessingResponse(int status, com.yahoo.processing.Request processingRequest,
+                              com.yahoo.processing.Response processingResponse,
+                              Renderer renderer,
+                              Executor renderingExecutor, Execution execution) {
         super(status);
         this.processingRequest = processingRequest;
         this.processingResponse = processingResponse;
-        this.renderingExecutor = renderingExecutor;
         this.execution = execution;
         this.renderer = renderer;
     }
 
     @SuppressWarnings("unchecked")
     @Override
-    public void render(final OutputStream stream, final ContentChannel channel,
-                       final CompletionHandler completionHandler) throws IOException {
+    public void render(OutputStream stream, ContentChannel channel, 
+                       CompletionHandler completionHandler) throws IOException {
         if (renderer instanceof AsynchronousRenderer) {
             AsynchronousRenderer asyncRenderer = (AsynchronousRenderer)renderer;
             asyncRenderer.setNetworkWiring(channel, completionHandler);
diff --git a/container-core/src/main/java/com/yahoo/processing/rendering/AsynchronousSectionedRenderer.java b/container-core/src/main/java/com/yahoo/processing/rendering/AsynchronousSectionedRenderer.java
index cc186ea21ee..eeb4a2ef36d 100644
--- a/container-core/src/main/java/com/yahoo/processing/rendering/AsynchronousSectionedRenderer.java
+++ b/container-core/src/main/java/com/yahoo/processing/rendering/AsynchronousSectionedRenderer.java
@@ -93,8 +93,8 @@ public abstract class AsynchronousSectionedRenderer<RESPONSE extends Response> e
 
     private static final Logger logger = Logger.getLogger(AsynchronousSectionedRenderer.class.getName());
 
-    // NOTE: Renderers are *prototype objects* - a new instance is created for each rendering by invoking clone
-    // calling init() and then render().
+    // NOTE: Renderers are *prototype objects* - a new instance is created for each rendering by invoking 
+    // clone(), init() and then render().
     // Hence any field which is not reinitialized in init() or render() will be *reused* in all rendering operations
     // across all threads!
 
@@ -110,8 +110,8 @@ public abstract class AsynchronousSectionedRenderer<RESPONSE extends Response> e
     // This MUST be created in the init() method - see comment above
     private Object singleThreaded;
 
-    // Rendering threads should never block.
-    // Burst traffic may add work faster than we can complete it, so use an unbounded queue.
+    // Rendering threads should never block so use one thread per core.
+    // We should complete any work we have already started so use an unbounded queue.
     // The executor SHOULD be reused across all instances having the same prototype
     private final ThreadPoolExecutor renderingExecutor = createExecutor();
     private static ThreadPoolExecutor createExecutor() {