Truly test application delete

7 files changed, 127 insertions, 45 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index 01e495630bd..c32fef58b01 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -105,7 +105,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         if (session == null) return false;
 
         NestedTransaction transaction = new NestedTransaction();
-        transaction.add(localSessionRepo.removeSessionTransaction(session.getSessionId()));
+        localSessionRepo.removeSession(session.getSessionId(), transaction);
         session.delete(transaction); // TODO: Not tested
 
         transaction.add(new Rotations(owner.get().getCurator(), owner.get().getPath()).delete(applicationId)); // TODO: Not tested
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
index a523f1dd662..771099ef163 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
@@ -10,6 +10,7 @@ import com.google.common.collect.HashMultiset;
 import com.google.common.collect.Multiset;
 import com.yahoo.log.LogLevel;
 import com.yahoo.path.Path;
+import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.config.server.application.ApplicationSet;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.yolean.Exceptions;
@@ -80,9 +81,44 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
         this.directoryCache = curator.createDirectoryCache(sessionsPath.getAbsolute(), false, false, executorService);
         this.directoryCache.start();
         this.directoryCache.addListener(this);
-        sessionsChanged(getSessionList(directoryCache.getCurrentData()));
+        sessionsChanged();
     }
 
+    //---------- START overrides to keep sessions changed in sync 
+
+    @Override
+    public synchronized void addSession(RemoteSession session) {
+        super.addSession(session);
+        sessionAdded(session.getSessionId());
+    }
+
+    @Override
+    public synchronized void removeSessionOrThrow(long id) {
+        super.removeSessionOrThrow(id);
+        sessionRemoved(id);
+    }
+
+    /**
+     * Removes a session
+     *
+     * @param id the id of the session to remove
+     * @return the removed session, or null if none was found
+     */
+    @Override
+    public synchronized RemoteSession removeSession(long id) { 
+        RemoteSession session = super.removeSession(id);
+        sessionRemoved(id);
+        return session;
+    }
+
+    @Override
+    public void removeSession(long id, NestedTransaction transaction) {
+        super.removeSession(id, transaction);
+        transaction.onCommitted(() -> sessionRemoved(id));
+    }
+
+    //---------- END overrides to keep sessions changed in sync 
+
     private void loadActiveSession(RemoteSession session) {
         tryReload(session.ensureApplicationLoaded(), session.logPre());
     }
@@ -115,30 +151,22 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
         return sessions;
     }
 
-    synchronized void sessionsChanged(List<Long> sessions) throws NumberFormatException {
+    synchronized void sessionsChanged() throws NumberFormatException {
+        List<Long> sessions = getSessionList(directoryCache.getCurrentData());
         checkForRemovedSessions(sessions);
         checkForAddedSessions(sessions);
     }
 
     private void checkForRemovedSessions(List<Long> sessions) {
-        for (RemoteSession session : listSessions()) {
-            if (!sessions.contains(session.getSessionId())) {
-                SessionStateWatcher watcher = sessionStateWatchers.remove(session.getSessionId());
-                watcher.close();
-                removeSessionOrThrow(session.getSessionId());
-                metrics.incRemovedSessions();
-            }
-        }
+        for (RemoteSession session : listSessions())
+            if ( ! sessions.contains(session.getSessionId()))
+                sessionRemoved(session.getSessionId());
     }
-
+    
     private void checkForAddedSessions(List<Long> sessions) {
-        for (Long sessionId : sessions) {
-            if (getSession(sessionId) == null) {
-                log.log(LogLevel.DEBUG, "Loading session id " + sessionId);
-                newSession(sessionId);
-                metrics.incAddedSessions();
-            }
-        }
+        for (Long sessionId : sessions)
+            if (getSession(sessionId) == null)
+                sessionAdded(sessionId);
     }
 
     /**
@@ -146,7 +174,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
      *
      * @param sessionId session id for the new session
      */
-    private void newSession(long sessionId) {
+    private void sessionAdded(long sessionId) {
         try {
             log.log(LogLevel.DEBUG, "Adding session to RemoteSessionRepo: " + sessionId);
             RemoteSession session = remoteSessionFactory.createSession(sessionId);
@@ -155,12 +183,20 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
             fileCache.addListener(this);
             loadSessionIfActive(session);
             sessionStateWatchers.put(sessionId, new SessionStateWatcher(fileCache, reloadHandler, session, metrics));
-            addSession(session);
+            internalAddSession(session);
+            metrics.incAddedSessions();
         } catch (Exception e) {
-            log.log(Level.WARNING, "Failed loading session " + sessionId + " (no config for this session can be served) : " + Exceptions.toMessageString(e));
+            log.log(Level.WARNING, "Failed loading session " + sessionId + ": No config for this session can be served", e);
         }
     }
 
+    private void sessionRemoved(long sessionId) {
+        SessionStateWatcher watcher = sessionStateWatchers.remove(sessionId);
+        watcher.close();
+        internalRemoveSessionOrThrow(sessionId);
+        metrics.incRemovedSessions();
+    }
+
     private void loadSessionIfActive(RemoteSession session) {
         for (ApplicationId applicationId : applicationRepo.listApplications()) {
             try {
@@ -206,11 +242,11 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
         }
         switch (event.getType()) {
             case CHILD_ADDED:
-                sessionsChanged(getSessionList(directoryCache.getCurrentData()));
+                sessionsChanged();
                 synchronizeOnNew(getSessionList(Collections.singletonList(event.getData())));
                 break;
             case CHILD_REMOVED:
-                sessionsChanged(getSessionList(directoryCache.getCurrentData()));
+                sessionsChanged();
                 break;
         }
     }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepo.java
index 2f97bea5962..1d8113f66ad 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepo.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepo.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.config.server.session;
 
 import com.yahoo.transaction.AbstractTransaction;
+import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.config.server.TimeoutBudget;
 import com.yahoo.vespa.config.server.http.NotFoundException;
@@ -24,32 +25,39 @@ public class SessionRepo<SESSIONTYPE extends Session> {
     private final HashMap<Long, SESSIONTYPE> sessions = new HashMap<>();
 
     public synchronized void addSession(SESSIONTYPE session) {
-        final long sessionId = session.getSessionId();
-        if (sessions.containsKey(sessionId)) {
-            throw new IllegalArgumentException("There already exists a session with id '" + sessionId + "'");
-        }
-        sessions.put(sessionId, session);
+        internalAddSession(session);
+    }
+
+    /** Why is this needed? Because of implementation inheritance - see RemoveSessionRepo */
+    protected synchronized final void internalAddSession(SESSIONTYPE session) {
+        if (sessions.containsKey(session.getSessionId()))
+            throw new IllegalArgumentException("There already exists a session with id '" + session.getSessionId() + "'");
+        sessions.put(session.getSessionId(), session);
     }
 
     public synchronized void removeSessionOrThrow(long id) {
-        if ( ! sessions.containsKey(id)) {
+        internalRemoveSessionOrThrow(id);
+    }
+
+    /** Why is this needed? Because of implementation inheritance - see RemoveSessionRepo */
+    protected synchronized final void internalRemoveSessionOrThrow(long id) {
+        if ( ! sessions.containsKey(id))
             throw new IllegalArgumentException("No such session exists '" + id + "'");
-        }
         sessions.remove(id);
     }
 
     /** 
-     * Removes a session
+     * Removes a session in a transaction
      * 
      * @param id the id of the session to remove
      * @return the removed session, or null if none was found
      */
     public synchronized SESSIONTYPE removeSession(long id) { return sessions.remove(id); }
     
-    public SessionRepoTransaction removeSessionTransaction(long id) {
+    public void removeSession(long id, NestedTransaction nestedTransaction) {
         SessionRepoTransaction transaction = new SessionRepoTransaction();
         transaction.addRemoveOperation(id);
-        return transaction;
+        nestedTransaction.add(transaction);
     }
 
     /**
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/MockReloadHandler.java b/configserver/src/test/java/com/yahoo/vespa/config/server/MockReloadHandler.java
index dddca55a2a4..6af557d9fe3 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/MockReloadHandler.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/MockReloadHandler.java
@@ -9,8 +9,8 @@ import com.yahoo.vespa.config.server.application.ApplicationSet;
  * @since 5.1.24
  */
 public class MockReloadHandler implements ReloadHandler {
+
     public ApplicationSet current = null;
-    public ReloadListener listener = null;
     public volatile ApplicationId lastRemoved = null;
 
     @Override
@@ -22,4 +22,5 @@ public class MockReloadHandler implements ReloadHandler {
     public void removeApplication(ApplicationId applicationId) {
         lastRemoved = applicationId;
     }
+
 }
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 4178fdcb0d3..63784c1acb0 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
@@ -251,7 +251,10 @@ public class ApplicationHandlerTest {
         
         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), null, 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();
@@ -268,7 +271,7 @@ public class ApplicationHandlerTest {
                                                     null,
                                                     new SuperModelGenerationCounter(curator));
         tenant.getLocalSessionRepo().addSession(new LocalSession(tenantName, sessionId, null, context));
-        sessionClient.writeApplicationId(applicationId); // TODO: Instead, use Applicationrepository to deploy the application
+        sessionClient.writeApplicationId(applicationId); // TODO: Instead, use ApplicationRepository to deploy the application
 
         tenant.getRemoteSessionRepo().addSession(new RemoteSession(tenantName, sessionId,
                                                                    new TestComponentRegistry(curator,
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MemoryFileSystem.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MemoryFileSystem.java
index 978827aebc2..2a598c8c4e0 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MemoryFileSystem.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MemoryFileSystem.java
@@ -92,6 +92,16 @@ class MemoryFileSystem extends FileSystem {
     public void replaceRoot(Node newRoot) { this.root = newRoot; }
 
     /**
+     * Lists the entire content of this file system as a multiline string.
+     * Useful for debugging.
+     */
+    public String dumpState() { 
+        StringBuilder b = new StringBuilder();
+        root.writeRecursivelyTo(b, "");
+        return b.toString();
+    }
+
+    /**
      * A node in this file system. Nodes may have children (a "directory"),
      * content (a "file"), or both.
      */
@@ -103,7 +113,7 @@ class MemoryFileSystem extends FileSystem {
         /** The local name of this node */
         private final String name;
 
-        /** The content of this node. This buffer is effectively immutable. */
+        /** The content of this node, never null. This buffer is effectively immutable. */
         private byte[] content;
 
         private Map<String, Node> children = new LinkedHashMap<>();
@@ -187,6 +197,24 @@ class MemoryFileSystem extends FileSystem {
         /** Returns an unmodifiable map of the immediate children of this indexed by their local name */
         public Map<String, Node> children() { return Collections.unmodifiableMap(children); }
 
+        /** 
+         * Dumps the content of this and all children recursively to the given string builder.
+         * Useful for debugging.
+         * 
+         * @param b the builder to write to
+         * @param pathPrefix, the path to this node, never ending by "/"
+         */
+        public void writeRecursivelyTo(StringBuilder b, String pathPrefix) {
+            String path = ( pathPrefix.equals("/") ? "" : pathPrefix ) + "/" + name;
+            b.append(path);
+            if (content.length > 0)
+                b.append(" [content: ").append(content.length).append(" bytes]");
+            b.append("\n");
+            
+            for (Node child : children.values())
+                child.writeRecursivelyTo(b, path);
+        }
+
         @Override
         public String toString() {
             return "directory '" + name() + "'";
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
index 9d347f8be7f..92db255a67f 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
@@ -130,6 +130,12 @@ public class MockCurator extends Curator {
         curatorFramework.start();
     }
 
+    /**
+     * Lists the entire content of this curator instance as a multiline string.
+     * Useful for debugging.
+     */
+    public String dumpState() { return fileSystem.dumpState(); }
+
     /** Returns a started curator framework */
     public CuratorFramework framework() { return curatorFramework; }
 
@@ -361,6 +367,7 @@ public class MockCurator extends Curator {
                 }
             }
             catch (Exception e) {
+                e.printStackTrace(); // TODO: Remove
                 throw new RuntimeException("Exception notifying listeners", e);
             }
         }
@@ -517,13 +524,12 @@ public class MockCurator extends Curator {
 
         @Override
         public List<ChildData> getCurrentData() {
-            Node parent = fileSystem.root().getNode(Paths.get(path.toString()), false);
-            if (parent == null) return Collections.emptyList(); // behavior in this case is unspecified
-
-            List<ChildData> data = new ArrayList<>();
-            collectData(parent, path, data);
-            Collections.sort(data);
-            return data;
+            List<ChildData> childData = new ArrayList<>();
+            for (String childName : getChildren(path)) {
+                Path childPath = path.append(childName);
+                childData.add(new ChildData(childPath.getAbsolute(), null, getData(childPath).get()));
+            }
+            return childData;
         }
 
         private void collectData(Node parent, Path parentPath, List<ChildData> data) {