diff options
author | Jon Bratseth <bratseth@yahoo-inc.com> | 2016-07-08 12:57:08 +0200 |
---|---|---|
committer | Jon Bratseth <bratseth@yahoo-inc.com> | 2016-07-08 12:57:08 +0200 |
commit | 55e1305da512f88b3825a413ab76695f2f7339ff (patch) | |
tree | 3df4611d0953f6d32cf857179e90b192f1046874 | |
parent | 08afd426d391fa66f51be424b1f6b2f593b8e116 (diff) |
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) { |