From 330263d3a13cd604fad1a8e8eb2beddab228c6ac Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Tue, 15 Aug 2023 11:16:00 +0200 Subject: Revert "Ignore sessions in state NEW when initializing" --- .../config/server/session/SessionRepository.java | 35 +++++++++------------- .../server/session/SessionRepositoryTest.java | 20 ------------- 2 files changed, 14 insertions(+), 41 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java index 65ac68ac234..f82aa405380 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java @@ -182,7 +182,7 @@ public class SessionRepository { loadSessions(executor); } - // non-private for testing + // For testing void loadSessions(ExecutorService executor) { loadRemoteSessions(executor); try { @@ -410,15 +410,14 @@ public class SessionRepository { } private void loadRemoteSessions(ExecutorService executor) throws NumberFormatException { - Map> futures = new HashMap<>(); + Map> futures = new HashMap<>(); for (long sessionId : getRemoteSessionsFromZooKeeper()) { - futures.put(sessionId, executor.submit(() -> addSession(sessionId, true))); + futures.put(sessionId, executor.submit(() -> sessionAdded(sessionId))); } futures.forEach((sessionId, future) -> { try { - boolean loaded = future.get(); - if (loaded) - log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded"); + future.get(); + log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded"); } catch (ExecutionException | InterruptedException e) { throw new RuntimeException("Could not load remote session " + sessionId, e); } @@ -426,32 +425,26 @@ public class SessionRepository { } /** - * Add session if it is not already added and is not in a state where it should be ignored. + * A session for which we don't have a watcher, i.e. hitherto unknown to us. * * @param sessionId session id for the new session - * @return true if added, false otherwise */ - public boolean addSession(long sessionId, boolean initializing) { - if (sessionShouldBeIgnored(sessionId, initializing)) return false; + public void sessionAdded(long sessionId) { + if (hasStatusDeleted(sessionId)) return; log.log(Level.FINE, () -> "Adding remote session " + sessionId); Session session = createRemoteSession(sessionId); - if (session.getStatus() == Session.Status.NEW) + if (session.getStatus() == Session.Status.NEW) { + log.log(Level.FINE, () -> session.logPre() + "Confirming upload for session " + sessionId); confirmUpload(session); + } createLocalSessionFromDistributedApplicationPackage(sessionId); - return true; } - private boolean sessionShouldBeIgnored(long sessionId, boolean initializing) { - var ignorableStates = new HashSet<>(Set.of(Session.Status.DELETE)); - // Also ignore sessions in NEW state when initializing, they are missing application package - // and other session data, usually because deployment failed - if (initializing) - ignorableStates.add(Session.Status.NEW); - + private boolean hasStatusDeleted(long sessionId) { SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId); RemoteSession session = new RemoteSession(tenantName, sessionId, sessionZKClient); - return ignorableStates.contains(session.getStatus()); + return session.getStatus() == Session.Status.DELETE; } void activate(long sessionId) { @@ -967,7 +960,7 @@ public class SessionRepository { private void checkForAddedSessions(List sessions) { for (Long sessionId : sessions) if (remoteSessionCache.get(sessionId) == null) - addSession(sessionId, false); + sessionAdded(sessionId); } public Transaction createActivateTransaction(Session session) { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java index fec4c096c7b..07d3aac5a52 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java @@ -83,10 +83,6 @@ public class SessionRepositoryTest { private void setup(ModelFactoryRegistry modelFactoryRegistry) throws Exception { curator = new MockCurator(); - setup(modelFactoryRegistry, curator); - } - - private void setup(ModelFactoryRegistry modelFactoryRegistry, MockCurator curator) throws Exception { File configserverDbDir = temporaryFolder.newFolder().getAbsoluteFile(); ConfigserverConfig configserverConfig = new ConfigserverConfig.Builder() .configServerDBDir(configserverDbDir.getAbsolutePath()) @@ -194,22 +190,6 @@ public class SessionRepositoryTest { assertTrue(sessionRepository.getRemoteSessionsFromZooKeeper().isEmpty()); } - // If reading a session that originally failed deployment we should just skip it - @Test - public void testThatSessionThatFailedDeploymentIsIgnored() throws Exception { - setup(); - - // Create a new session, which will have status NEW, which is what will be the state if e.g. - // a deployment failed when building the config model - createSession(3, false); - - // setup() will load remote sessions, session 3 should not be loaded, since it is in state NEW - setup(new ModelFactoryRegistry(List.of(VespaModelFactory.createTestFactory())), curator); - assertTrue(sessionRepository.getRemoteSessions().isEmpty()); - Path sessionPath = TenantRepository.getSessionsPath(tenantName).append(3 + ""); - assertTrue(curator.exists(sessionPath)); - } - @Test public void require_that_new_invalid_application_throws_exception() throws Exception { MockModelFactory failingFactory = new MockModelFactory(); -- cgit v1.2.3