Improve logging when removing an application

Log only when it is removed, add some more validation

9 files changed, 55 insertions, 57 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationMapper.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationMapper.java
index c0158b55422..ecdd2357b1f 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationMapper.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationMapper.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.application;
 
 import com.yahoo.component.Version;
@@ -24,12 +24,10 @@ public final class ApplicationMapper {
     private final Map<ApplicationId, ApplicationSet> requestHandlers = new ConcurrentHashMap<>();
 
     private ApplicationSet getApplicationSet(ApplicationId applicationId) {
-        ApplicationSet list = requestHandlers.get(applicationId);
-        if (list != null) {
-            return list;
-        }
+        ApplicationSet set = requestHandlers.get(applicationId);
+        if (set == null) throw new NotFoundException("No such application id: " + applicationId);
 
-        throw new NotFoundException("No such application id: " + applicationId);
+        return set;
     }
 
     /**
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationSet.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationSet.java
index 12353c0e7e3..a9aaedd0a0e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationSet.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationSet.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.application;
 
 import com.yahoo.config.model.api.HostInfo;
@@ -17,21 +17,29 @@ import java.util.stream.Collectors;
 public final class ApplicationSet {
 
     private final Version latestVersion;
-    // TODO: Should not need these as part of application?
     private final ApplicationId applicationId;
     private final long generation;
     private final HashMap<Version, Application> applications = new HashMap<>();
 
-    private ApplicationSet(List<Application> applicationList) {
-        applicationId = applicationList.get(0).getId();
-        generation = applicationList.get(0).getApplicationGeneration();
-        for (Application application : applicationList) {
-            applications.put(application.getVespaVersion(), application);
-            if (!application.getId().equals(applicationId)) {
-                throw new IllegalArgumentException("Trying to create set with different application ids");
+    private ApplicationSet(List<Application> applications) {
+        if (applications.isEmpty()) throw new IllegalArgumentException("application list cannot be empty");
+
+        Application firstApp = applications.get(0);
+        applicationId = firstApp.getId();
+        generation = firstApp.getApplicationGeneration();
+        for (Application application : applications) {
+            this.applications.put(application.getVespaVersion(), application);
+            ApplicationId applicationId = application.getId();
+            if ( ! applicationId.equals(this.applicationId)) {
+                throw new IllegalArgumentException("Trying to create set with different application ids (" +
+                                                   application + " and " + this.applicationId + ")");
+            }
+            if ( ! application.getApplicationGeneration().equals(generation)) {
+                throw new IllegalArgumentException("Trying to create set with different generations ("  +
+                                                   generation + " and " + this.generation + ")");
             }
         }
-        latestVersion = applications.keySet().stream().max((a, b) -> a.compareTo(b)).get();
+        latestVersion = this.applications.keySet().stream().max(Version::compareTo).get();
     }
 
     /**
@@ -71,8 +79,8 @@ public final class ApplicationSet {
         return new ApplicationSet(applications);
     }
 
-    public static ApplicationSet fromSingle(Application application) {
-        return fromList(Arrays.asList(application));
+    public static ApplicationSet from(Application application) {
+        return fromList(List.of(application));
     }
 
     public Collection<String> getAllHosts() {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
index 6e87b65dcb8..f07a595a830 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.application;
 
 import com.yahoo.cloud.config.ConfigserverConfig;
@@ -8,7 +8,6 @@ import com.yahoo.config.FileReference;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.path.Path;
-import com.yahoo.text.Utf8;
 import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.config.ConfigKey;
 import com.yahoo.vespa.config.GetConfigRequest;
@@ -26,7 +25,6 @@ import com.yahoo.vespa.config.server.rpc.ConfigResponseFactory;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.Lock;
-import com.yahoo.vespa.curator.transaction.CuratorOperations;
 import com.yahoo.vespa.curator.transaction.CuratorTransaction;
 import org.apache.curator.framework.CuratorFramework;
 import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
@@ -34,7 +32,6 @@ import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.time.Clock;
-import java.time.Duration;
 import java.util.Collection;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -44,7 +41,6 @@ import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 import static java.util.stream.Collectors.toSet;
 
@@ -187,13 +183,16 @@ public class TenantApplications implements RequestHandler, HostValidator<Applica
 
     private void childEvent(CuratorFramework ignored, PathChildrenCacheEvent event) {
         zkWatcherExecutor.execute(() -> {
+            ApplicationId applicationId = ApplicationId.fromSerializedForm(Path.fromString(event.getData().getPath()).getName());
             switch (event.getType()) {
                 case CHILD_ADDED:
-                    applicationAdded(ApplicationId.fromSerializedForm(Path.fromString(event.getData().getPath()).getName()));
+                    /* A new application is added when a session is added, @see
+                    {@link com.yahoo.vespa.config.server.session.SessionRepository#childEvent(CuratorFramework, PathChildrenCacheEvent)} */
+                    log.log(Level.FINE, TenantRepository.logPre(applicationId) + "Application added: " + applicationId);
                     break;
                 // Event CHILD_REMOVED will be triggered on all config servers if deleteApplication() above is called on one of them
                 case CHILD_REMOVED:
-                    applicationRemoved(ApplicationId.fromSerializedForm(Path.fromString(event.getData().getPath()).getName()));
+                    removeApplication(applicationId);
                     break;
                 case CHILD_UPDATED:
                     // do nothing, application just got redeployed
@@ -202,20 +201,10 @@ public class TenantApplications implements RequestHandler, HostValidator<Applica
                     break;
             }
             // We may have lost events and may need to remove applications.
-            // New applications are added when session is added, not here. See SessionRepository.
             removeUnusedApplications();
         });
     }
 
-    private void applicationRemoved(ApplicationId applicationId) {
-        removeApplication(applicationId);
-        log.log(Level.INFO, TenantRepository.logPre(applicationId) + "Application removed: " + applicationId);
-    }
-
-    private void applicationAdded(ApplicationId applicationId) {
-        log.log(Level.FINE, TenantRepository.logPre(applicationId) + "Application added: " + applicationId);
-    }
-
     /**
      * Gets a config for the given app, or null if not found
      */
@@ -253,8 +242,10 @@ public class TenantApplications implements RequestHandler, HostValidator<Applica
 
     public void removeApplication(ApplicationId applicationId) {
         try (Lock lock = lock(applicationId)) {
-            if (exists(applicationId))
-                return; // Application was deployed again.
+            if (exists(applicationId)) {
+                log.log(Level.INFO, "Tried removing application " + applicationId + ", but it seems to have been deployed again");
+                return;
+            }
 
             if (applicationMapper.hasApplication(applicationId, clock.instant())) {
                 applicationMapper.remove(applicationId);
@@ -262,6 +253,7 @@ public class TenantApplications implements RequestHandler, HostValidator<Applica
                 reloadListenersOnRemove(applicationId);
                 tenantMetricUpdater.setApplications(applicationMapper.numApplications());
                 metrics.removeMetricUpdater(Metrics.createDimensions(applicationId));
+                log.log(Level.INFO, "Application removed: " + applicationId);
             }
         }
     }
@@ -269,7 +261,6 @@ public class TenantApplications implements RequestHandler, HostValidator<Applica
     public void removeApplicationsExcept(Set<ApplicationId> applications) {
         for (ApplicationId activeApplication : applicationMapper.listApplicationIds()) {
             if ( ! applications.contains(activeApplication)) {
-                log.log(Level.INFO, "Will remove deleted application " + activeApplication.toShortString());
                 removeApplication(activeApplication);
             }
         }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/SuperModelRequestHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/SuperModelRequestHandlerTest.java
index f91205af44d..26ef180cf8d 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/SuperModelRequestHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/SuperModelRequestHandlerTest.java
@@ -114,7 +114,7 @@ public class SuperModelRequestHandlerTest {
     }
 
     private ApplicationSet createApp(ApplicationId applicationId, long generation) throws IOException, SAXException {
-        return ApplicationSet.fromSingle(
+        return ApplicationSet.from(
                 new TestApplication(
                         new VespaModel(FilesApplicationPackage.fromFile(testApp)),
                         new ServerCache(),
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationMapperTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationMapperTest.java
index 4a55d013584..3270bb5cd76 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationMapperTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationMapperTest.java
@@ -58,7 +58,7 @@ public class ApplicationMapperTest {
 
     @Test (expected = NotFoundException.class)
     public void testGetForVersionThrows2() {
-        applicationMapper.register(appId, ApplicationSet.fromSingle(applications.get(0)));
+        applicationMapper.register(appId, ApplicationSet.from(applications.get(0)));
 
         applicationMapper.getForVersion(new ApplicationId.Builder()
                                                 .tenant("different")
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java
index 0f6aa66b9c3..aa3f240b12e 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java
@@ -178,13 +178,13 @@ public class TenantApplicationsTest {
         ApplicationId applicationId = ApplicationId.defaultId();
         applications.createApplication(applicationId);
         applications.createPutTransaction(applicationId, 1).commit();
-        applications.activateApplication(ApplicationSet.fromSingle(new Application(model,
-                                                                                   new ServerCache(),
-                                                                                   1,
-                                                                                   false,
-                                                                                   vespaVersion,
-                                                                                   MetricUpdater.createTestUpdater(),
-                                                                                   applicationId)),
+        applications.activateApplication(ApplicationSet.from(new Application(model,
+                                                                             new ServerCache(),
+                                                                             1,
+                                                                             false,
+                                                                             vespaVersion,
+                                                                             MetricUpdater.createTestUpdater(),
+                                                                             applicationId)),
                                          1);
         Set<ConfigKey<?>> configNames = applications.listConfigs(applicationId, Optional.of(vespaVersion), false);
         assertTrue(configNames.contains(new ConfigKey<>("sentinel", "hosts", "cloud.config")));
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/RpcServerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/RpcServerTest.java
index 57462c41db8..704b00c4e0b 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/RpcServerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/rpc/RpcServerTest.java
@@ -135,7 +135,7 @@ public class RpcServerTest {
                                           new Version(1, 2, 3),
                                           MetricUpdater.createTestUpdater(),
                                           applicationId);
-        ApplicationSet appSet = ApplicationSet.fromSingle(app);
+        ApplicationSet appSet = ApplicationSet.from(app);
         tester.rpcServer().configActivated(appSet);
         ConfigKey<?> key = new ConfigKey<>(LbServicesConfig.class, "*");
         JRTClientConfigRequest clientReq  = createRequest(new RawConfig(key, LbServicesConfig.getDefMd5()));
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
index 6104d95b5c2..d9d6fb01086 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
@@ -89,13 +89,13 @@ public class TenantRepositoryTest {
         ApplicationId id = ApplicationId.from(tenant1, ApplicationName.defaultName(), InstanceName.defaultName());
         applicationRepo.createApplication(id);
         applicationRepo.createPutTransaction(id, 4).commit();
-        applicationRepo.activateApplication(ApplicationSet.fromSingle(new Application(new VespaModel(MockApplicationPackage.createEmpty()),
-                                                                                      new ServerCache(),
-                                                                                      4L,
-                                                                                      false,
-                                                                                      new Version(1, 2, 3),
-                                                                                      MetricUpdater.createTestUpdater(),
-                                                                                      id)),
+        applicationRepo.activateApplication(ApplicationSet.from(new Application(new VespaModel(MockApplicationPackage.createEmpty()),
+                                                                                new ServerCache(),
+                                                                                4L,
+                                                                                false,
+                                                                                new Version(1, 2, 3),
+                                                                                MetricUpdater.createTestUpdater(),
+                                                                                id)),
                                             4);
         assertEquals(1, listener.reloaded.get());
     }
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/CuratorCompletionWaiter.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/CuratorCompletionWaiter.java
index a3a15375d40..fb8c303db66 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/CuratorCompletionWaiter.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/CuratorCompletionWaiter.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.curator;
 
 import java.util.logging.Level;
@@ -43,7 +43,8 @@ class CuratorCompletionWaiter implements Curator.CompletionWaiter {
             throw new RuntimeException(e);
         }
         if (respondents.size() < barrierMemberCount()) {
-            throw new CompletionTimeoutException("Timed out waiting for peer config servers to complete operation. " +
+            throw new CompletionTimeoutException("Timed out waiting for peer config servers to complete operation " +
+                                                 "(waited for barrier " + barrierPath + ")." +
                                                  "Got response from " + respondents + ", but need response from " +
                                                  "at least " + barrierMemberCount() + " server(s). " +
                                                  "Timeout passed as argument was " + timeout.toMillis() + " ms");