Merge pull request #3946 from vespa-engine/hmusum/tenants-cleanup

Cleanup and make Tenants a bit more readable

7 files changed, 60 insertions, 73 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/TenantHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/TenantHandler.java
index b02d17a9372..46d007126c0 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/TenantHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/TenantHandler.java
@@ -37,7 +37,7 @@ public class TenantHandler extends HttpHandler {
     protected HttpResponse handlePUT(HttpRequest request) {
         TenantName tenantName = getAndValidateTenantFromRequest(request);
         try {
-            tenants.writeTenantPath(tenantName);
+            tenants.addTenant(tenantName);
         } catch (Exception e) {
             throw new InternalServerException(Exceptions.toMessageString(e));
         }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java
index 3ad920bbff2..cf05440a1d2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java
@@ -28,7 +28,6 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -42,7 +41,7 @@ import java.util.logging.Logger;
  * implemented support for it).
  *
  * This instance is called from two different threads, the http handler threads and the zookeeper watcher threads.
- * To create or delete a tenant, the handler calls {@link Tenants#writeTenantPath} and {@link Tenants#deleteTenant} methods.
+ * To create or delete a tenant, the handler calls {@link Tenants#addTenant} and {@link Tenants#deleteTenant} methods.
  * This will delete shared state from zookeeper, and return, so it does not mean a tenant is immediately deleted.
  *
  * Once a tenant is deleted from zookeeper, the zookeeper watcher thread will get notified on all configservers, and
@@ -73,14 +72,13 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
 
 
     /**
-     * New instance from the tenants in the given component registry's ZooKeeper. Will set watch when reading them.
+     * New instance from the tenants in the given component registry's ZooKeeper data.
      * 
      * @param globalComponentRegistry a {@link com.yahoo.vespa.config.server.GlobalComponentRegistry}
      * @throws Exception is creating the Tenants instance fails
      */
     @Inject
     public Tenants(GlobalComponentRegistry globalComponentRegistry, Metrics metrics) throws Exception {
-        // Note: unit tests may want to use the constructor below to avoid setting watch by calling readTenants().
         this.globalComponentRegistry = globalComponentRegistry;
         this.curator = globalComponentRegistry.getCurator();
         metricUpdater = metrics.getOrCreateMetricUpdater(Collections.emptyMap());
@@ -94,12 +92,12 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
         this.directoryCache = curator.createDirectoryCache(tenantsPath.getAbsolute(), false, false, pathChildrenExecutor);
         directoryCache.start();
         directoryCache.addListener(this);
-        tenantsChanged(readTenants());
+        createTenants();
         notifyTenantsLoaded();
     }
 
     /**
-     * New instance containing the given tenants. This will not watch in ZooKeeper. 
+     * New instance containing the given tenants. This will not create Zookeeper watches. For testing only
      * @param globalComponentRegistry a {@link com.yahoo.vespa.config.server.GlobalComponentRegistry} instance
      * @param metrics a {@link com.yahoo.vespa.config.server.monitoring.Metrics} instance
      * @param tenants a collection of {@link Tenant}s
@@ -131,9 +129,9 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
         return sessionTenants;
     }
     
-    public synchronized void addTenant(Tenant tenant) {
-        tenants.put(tenant.getName(), tenant);
-        metricUpdater.setTenants(tenants.size());
+    public synchronized void addTenant(TenantName tenantName) throws Exception {
+        writeTenantPath(tenantName);
+        createTenant(tenantName);
     }
 
     /**
@@ -141,7 +139,7 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
      *
      * @return a set of tenant names
      */
-    private Set<TenantName> readTenants() {
+    private Set<TenantName> readTenantsFromZooKeeper() {
         Set<TenantName> tenants = new LinkedHashSet<>();
         for (String tenant : curator.getChildren(tenantsPath)) {
             tenants.add(TenantName.from(tenant));
@@ -149,10 +147,11 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
         return tenants;
     }
 
-    synchronized void tenantsChanged(Set<TenantName> newTenants) throws Exception {
-        log.log(LogLevel.DEBUG, "Tenants changed: " + newTenants);
-        checkForRemovedTenants(newTenants);
-        checkForAddedTenants(newTenants);
+    synchronized void createTenants() throws Exception {
+        Set<TenantName> allTenants = readTenantsFromZooKeeper();
+        log.log(LogLevel.DEBUG, "Create tenants, tenants found in zookeeper: " + allTenants);
+        checkForRemovedTenants(allTenants);
+        checkForAddedTenants(allTenants);
         metricUpdater.setTenants(tenants.size());
     }
 
@@ -170,24 +169,26 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
 
     private void checkForAddedTenants(Set<TenantName> newTenants) throws Exception {
         ExecutorService executor = Executors.newFixedThreadPool(globalComponentRegistry.getConfigserverConfig().numParallelTenantLoaders());
-        Map<TenantName, Tenant> addedTenants = new ConcurrentHashMap<>();
         for (TenantName tenantName : newTenants) {
             // Note: the http handler will check if the tenant exists, and throw accordingly
             if (!tenants.containsKey(tenantName)) {
                 executor.execute(() -> {
-                    try {
-                        Tenant tenant = TenantBuilder.create(globalComponentRegistry, tenantName, getTenantPath(tenantName)).build();
-                        notifyNewTenant(tenant);
-                        addedTenants.put(tenantName, tenant);
-                    } catch (Exception e) {
-                        log.log(LogLevel.WARNING, "Error loading tenant '" + tenantName + "', skipping.", e);
-                    }
+                    createTenant(tenantName);
                 });
             }
         }
         executor.shutdown();
         executor.awaitTermination(365, TimeUnit.DAYS); // Timeout should never happen
-        tenants.putAll(addedTenants);
+    }
+
+    private void createTenant(TenantName tenantName) {
+        try {
+            Tenant tenant = TenantBuilder.create(globalComponentRegistry, tenantName, getTenantPath(tenantName)).build();
+            notifyNewTenant(tenant);
+            tenants.put(tenantName, tenant);
+        } catch (Exception e) {
+            log.log(LogLevel.WARNING, "Error loading tenant '" + tenantName + "', skipping.", e);
+        }
     }
 
     /**
@@ -239,7 +240,7 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
      * @param name name of the tenant
      * @return this Tenants
      */
-    public synchronized Tenants writeTenantPath(TenantName name) {
+    synchronized Tenants writeTenantPath(TenantName name) {
         Path tenantPath = getTenantPath(name);
         curator.createAtomically(tenantPath, tenantPath.append(Tenant.SESSIONS), tenantPath.append(Tenant.APPLICATIONS));
         return this;
@@ -316,7 +317,7 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen
         switch (event.getType()) {
             case CHILD_ADDED:
             case CHILD_REMOVED:
-                tenantsChanged(readTenants());
+                createTenants();
                 break;
         }
     }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
index 88d7177a51e..e0d65055f21 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ConfigServerBootstrapTest.java
@@ -47,9 +47,9 @@ public class ConfigServerBootstrapTest extends TestWithTenant {
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
     @Before
-    public void setup() {
-        tenants.writeTenantPath(tenant1);
-        tenants.writeTenantPath(tenant2);
+    public void setup() throws Exception {
+        tenants.addTenant(tenant1);
+        tenants.addTenant(tenant2);
 
         applicationRepository = new ApplicationRepository(tenants,
                                                           new SessionHandlerTest.MockProvisioner(),
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
index afe51adee20..892c821950e 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
@@ -24,8 +24,6 @@ import com.yahoo.vespa.config.server.TestComponentRegistry;
 import com.yahoo.vespa.config.server.application.ApplicationConvergenceChecker;
 import com.yahoo.vespa.config.server.application.HttpProxy;
 import com.yahoo.vespa.config.server.application.LogServerLogGrabber;
-import com.yahoo.vespa.config.server.application.TenantApplications;
-import com.yahoo.vespa.config.server.application.ZKTenantApplications;
 import com.yahoo.vespa.config.server.http.HandlerTest;
 import com.yahoo.vespa.config.server.http.HttpErrorResponse;
 import com.yahoo.vespa.config.server.http.StaticResponse;
@@ -40,7 +38,6 @@ import com.yahoo.vespa.config.server.session.RemoteSession;
 import com.yahoo.vespa.config.server.session.SessionContext;
 import com.yahoo.vespa.config.server.session.SessionZooKeeperClient;
 import com.yahoo.vespa.config.server.tenant.Tenant;
-import com.yahoo.vespa.config.server.tenant.TenantBuilder;
 import com.yahoo.vespa.config.server.tenant.Tenants;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.model.VespaModelFactory;
@@ -279,7 +276,7 @@ public class ApplicationHandlerTest {
         tenant.getRemoteSessionRepo().addSession(new RemoteSession(tenant.getName(), sessionId, componentRegistry, new MockSessionZKClient(app), clock));
     }
 
-    static Tenants addApplication(ApplicationId applicationId, long sessionId) throws Exception {
+    private static Tenants addApplication(ApplicationId applicationId, long sessionId) throws Exception {
         // This method is a good illustration of the spaghetti wiring resulting from no design
         // TODO: When this setup looks sane we have refactored sufficiently that there is a design
 
@@ -289,18 +286,15 @@ public class ApplicationHandlerTest {
         Path sessionPath = tenantPath.append(Tenant.SESSIONS).append(String.valueOf(sessionId));
 
         MockCurator curator = new MockCurator();
-        GlobalComponentRegistry globalComponents = new TestComponentRegistry.Builder().curator(curator).build();
+        GlobalComponentRegistry componentRegistry = new TestComponentRegistry.Builder()
+                .curator(curator)
+                .modelFactoryRegistry(new ModelFactoryRegistry(
+                        Collections.singletonList(new VespaModelFactory(new NullConfigModelRegistry()))))
+                .build();
         
-        Tenants tenants = new Tenants(globalComponents, Metrics.createTestMetrics()); // Creates the application path element in zk
-        tenants.writeTenantPath(tenantName);
-        TenantApplications tenantApplications = ZKTenantApplications.create(curator, 
-                                                                            tenantPath.append(Tenant.APPLICATIONS), 
-                                                                            new MockReloadHandler(), // TODO: Use the real one
-                                                                            tenantName);
-        Tenant tenant = TenantBuilder.create(globalComponents, applicationId.tenant(), tenantPath)
-                                     .withApplicationRepo(tenantApplications)
-                                     .build();
-        tenants.addTenant(tenant);
+        Tenants tenants = new Tenants(componentRegistry, Metrics.createTestMetrics()); // Creates the application path element in zk
+        tenants.addTenant(tenantName);
+        Tenant tenant = tenants.getTenant(tenantName);
 
         tenant.getApplicationRepo().createPutApplicationTransaction(applicationId, sessionId).commit();
         ApplicationPackage app = FilesApplicationPackage.fromFile(testApp);
@@ -317,11 +311,7 @@ public class ApplicationHandlerTest {
 
         tenant.getRemoteSessionRepo().addSession(
                 new RemoteSession(tenantName, sessionId,
-                                  new TestComponentRegistry.Builder()
-                                          .curator(curator)
-                                          .modelFactoryRegistry(new ModelFactoryRegistry(
-                                                  Collections.singletonList(new VespaModelFactory(new NullConfigModelRegistry()))))
-                                          .build(),
+                                  componentRegistry,
                                   sessionClient,
                                   Clock.systemUTC()));
         return tenants;
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ListTenantsTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ListTenantsTest.java
index 189314577e4..aa27dd0783c 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ListTenantsTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ListTenantsTest.java
@@ -19,9 +19,9 @@ public class ListTenantsTest extends TenantTest {
 
     @Test
     public void testListTenants() throws Exception {
-        tenants.writeTenantPath(a);
-        tenants.writeTenantPath(b);
-        tenants.writeTenantPath(c);
+        tenants.addTenant(a);
+        tenants.addTenant(b);
+        tenants.addTenant(c);
 
         ListTenantsHandler listTenantsHandler = new ListTenantsHandler(testExecutor(), null, tenants);
         
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TenantHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TenantHandlerTest.java
index cf79770ad20..ca545611e7f 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TenantHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TenantHandlerTest.java
@@ -54,7 +54,7 @@ public class TenantHandlerTest extends TenantTest {
  
     @Test
     public void testGetExisting() throws Exception {
-        tenants.writeTenantPath(a);
+        tenants.addTenant(a);
         TenantGetResponse response = (TenantGetResponse) handler.handleGET(
                 HttpRequest.createTestRequest("http://deploy.example.yahoo.com:80/application/v2/tenant/a", Method.GET));
         assertResponseEquals(response, "{\"message\":\"Tenant 'a' exists.\"}");
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantsTestCase.java b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantsTestCase.java
index a77b7615e6d..ce16ac14f60 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantsTestCase.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantsTestCase.java
@@ -3,8 +3,6 @@ package com.yahoo.vespa.config.server.tenant;
 
 import com.yahoo.config.model.test.MockApplicationPackage;
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.ApplicationName;
-import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Version;
 import com.yahoo.vespa.config.server.application.ApplicationSet;
@@ -12,7 +10,6 @@ import com.yahoo.vespa.config.server.ServerCache;
 import com.yahoo.vespa.config.server.TestComponentRegistry;
 import com.yahoo.vespa.config.server.TestWithCurator;
 import com.yahoo.vespa.config.server.application.Application;
-import com.yahoo.vespa.config.server.deploy.MockDeployer;
 import com.yahoo.vespa.config.server.monitoring.MetricUpdater;
 import com.yahoo.vespa.config.server.monitoring.Metrics;
 import com.yahoo.vespa.model.VespaModel;
@@ -52,8 +49,8 @@ public class TenantsTestCase extends TestWithCurator {
         tenantListener.tenantsLoaded = false;
         tenants = new Tenants(globalComponentRegistry, Metrics.createTestMetrics());
         assertTrue(tenantListener.tenantsLoaded);
-        tenants.writeTenantPath(tenant1);
-        tenants.writeTenantPath(tenant2);
+        tenants.addTenant(tenant1);
+        tenants.addTenant(tenant2);
     }
 
     @After
@@ -79,7 +76,7 @@ public class TenantsTestCase extends TestWithCurator {
 
     @Test
     public void testTenantListenersNotified() throws Exception {
-        tenants.writeTenantPath(tenant3);
+        tenants.addTenant(tenant3);
         assertThat("tenant3 not the last created tenant. Tenants: " + tenants.getAllTenantNames() + ", /config/v2/tenants: " + readZKChildren("/config/v2/tenants"), tenantListener.tenantCreatedName, is(tenant3));
         tenants.deleteTenant(tenant2);
         assertFalse(tenants.getAllTenantNames().contains(tenant2));
@@ -91,7 +88,7 @@ public class TenantsTestCase extends TestWithCurator {
         Set<TenantName> allTenants = tenants.getAllTenantNames();
         assertTrue(allTenants.contains(tenant1));
         assertTrue(allTenants.contains(tenant2));
-        tenants.writeTenantPath(tenant3);
+        tenants.addTenant(tenant3);
         allTenants = tenants.getAllTenantNames();
         assertTrue(allTenants.contains(tenant1));
         assertTrue(allTenants.contains(tenant2));
@@ -100,7 +97,7 @@ public class TenantsTestCase extends TestWithCurator {
 
     @Test
     public void testPutAdd() throws Exception {
-        tenants.writeTenantPath(tenant3);
+        tenants.addTenant(tenant3);
         assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenants.tenantZkPath(tenant3)));
     }
     
@@ -115,29 +112,28 @@ public class TenantsTestCase extends TestWithCurator {
     public void testTenantsChanged() throws Exception {
         tenants.close(); // close the Tenants instance created in setupSession, we do not want to use one with a PatchChildrenCache listener
         tenants = new Tenants(globalComponentRegistry, Metrics.createTestMetrics(), new ArrayList<>());
-        Set<TenantName> newTenants = new LinkedHashSet<>();
         TenantName defaultTenant = TenantName.defaultName();
-        newTenants.add(tenant2);
-        newTenants.add(defaultTenant);
-        tenants.tenantsChanged(newTenants);
+        tenants.writeTenantPath(tenant2);
+        tenants.writeTenantPath(defaultTenant);
+        tenants.createTenants();
         Set<TenantName> allTenants = tenants.getAllTenantNames();
         assertTrue(allTenants.contains(tenant2));
         assertEquals("default", defaultTenant.value());
         assertTrue(allTenants.contains(defaultTenant));
-        assertFalse(allTenants.contains(tenant1));
-        newTenants.clear();
-        tenants.tenantsChanged(newTenants);
+        tenants.deleteTenant(tenant1);
+        tenants.deleteTenant(tenant2);
+        tenants.deleteTenant(defaultTenant);
+        tenants.createTenants();
         allTenants = tenants.getAllTenantNames();
         assertFalse(allTenants.contains(tenant1));
         assertFalse(allTenants.contains(tenant2));
         assertFalse(allTenants.contains(defaultTenant));
-        newTenants.clear();
         TenantName foo = TenantName.from("foo");
         TenantName bar = TenantName.from("bar");
-        newTenants.add(tenant2);
-        newTenants.add(foo);
-        newTenants.add(bar);
-        tenants.tenantsChanged(newTenants);
+        tenants.writeTenantPath(tenant2);
+        tenants.writeTenantPath(foo);
+        tenants.writeTenantPath(bar);
+        tenants.createTenants();
         allTenants = tenants.getAllTenantNames();
         assertTrue(allTenants.contains(tenant2));
         assertTrue(allTenants.contains(foo));