Merge pull request #14385 from vespa-engine/hmusum/configserver-refactoring-28

Simplify session state watcher

4 files changed, 23 insertions, 44 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
index 7fc6b35722f..6d2ef4028c6 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
@@ -4,7 +4,6 @@ package com.yahoo.vespa.config.server.modelfactory;
 import com.google.common.collect.ImmutableSet;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.ApplicationPackage;
-import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ConfigDefinitionRepo;
 import com.yahoo.config.model.api.ModelContext;
 import com.yahoo.config.model.api.ModelFactory;
@@ -57,7 +56,6 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
     private final ConfigDefinitionRepo configDefinitionRepo;
     private final Metrics metrics;
     private final Curator curator;
-    private final DeployLogger logger;
     private final FlagSource flagSource;
     private final SecretStore secretStore;
 
@@ -76,7 +74,6 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
         this.configDefinitionRepo = globalComponentRegistry.getStaticConfigDefinitionRepo();
         this.metrics = globalComponentRegistry.getMetrics();
         this.curator = globalComponentRegistry.getCurator();
-        this.logger = new SilentDeployLogger();
         this.flagSource = globalComponentRegistry.getFlagSource();
         this.secretStore = globalComponentRegistry.getSecretStore();
     }
@@ -90,14 +87,14 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
                                             Optional<AllocatedHosts> ignored // Ignored since we have this in the app package for activated models
     ) {
         log.log(Level.FINE, String.format("Loading model version %s for session %s application %s",
-                                              modelFactory.version(), appGeneration, applicationId));
+                                          modelFactory.version(), appGeneration, applicationId));
         ModelContext.Properties modelContextProperties = createModelContextProperties(applicationId);
         Provisioned provisioned = new Provisioned();
         ModelContext modelContext = new ModelContextImpl(
                 applicationPackage,
                 Optional.empty(),
                 permanentApplicationPackage.applicationPackage(),
-                logger,
+                new SilentDeployLogger(),
                 configDefinitionRepo,
                 getForVersionOrLatest(applicationPackage.getFileRegistries(), modelFactory.version()).orElse(new MockFileRegistry()),
                 createStaticProvisioner(applicationPackage.getAllocatedHosts(),
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionCache.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionCache.java
index b7d78f11201..60fa037e99a 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionCache.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionCache.java
@@ -15,20 +15,14 @@ public class SessionCache<SESSIONTYPE extends Session> {
 
     private final HashMap<Long, SESSIONTYPE> sessions = new HashMap<>();
 
-    public synchronized void addSession(SESSIONTYPE session) {
-        sessions.putIfAbsent(session.getSessionId(), session);
+    public synchronized void putSession(SESSIONTYPE session) {
+        sessions.put(session.getSessionId(), session);
     }
 
     synchronized void removeSession(long id) {
         sessions.remove(id);
     }
 
-    /**
-     * Gets a Session
-     *
-     * @param id session id
-     * @return a session belonging to the id supplied, or null if no session with the id was found
-     */
     public synchronized SESSIONTYPE getSession(long id) {
         return sessions.get(id);
     }
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 cbfa59b26e4..a9bab3ffdf8 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
@@ -121,10 +121,10 @@ public class SessionRepository {
     // ---------------- Local sessions ----------------------------------------------------------------
 
     public synchronized void addLocalSession(LocalSession session) {
-        localSessionCache.addSession(session);
+        localSessionCache.putSession(session);
         long sessionId = session.getSessionId();
         RemoteSession remoteSession = createRemoteSession(sessionId);
-        addSessionStateWatcher(sessionId, remoteSession, Optional.of(session));
+        addSessionStateWatcher(sessionId, remoteSession);
     }
 
     public LocalSession getLocalSession(long sessionId) {
@@ -263,7 +263,7 @@ public class SessionRepository {
     }
 
     public void addRemoteSession(RemoteSession session) {
-        remoteSessionCache.addSession(session);
+        remoteSessionCache.putSession(session);
         metrics.incAddedSessions();
     }
 
@@ -348,10 +348,9 @@ public class SessionRepository {
         RemoteSession remoteSession = createRemoteSession(sessionId);
         loadSessionIfActive(remoteSession);
         addRemoteSession(remoteSession);
-        Optional<LocalSession> localSession = Optional.empty();
         if (distributeApplicationPackage())
-            localSession = createLocalSessionUsingDistributedApplicationPackage(sessionId);
-        addSessionStateWatcher(sessionId, remoteSession, localSession);
+            createLocalSessionUsingDistributedApplicationPackage(sessionId);
+        addSessionStateWatcher(sessionId, remoteSession);
     }
 
     void activate(RemoteSession session) {
@@ -370,8 +369,10 @@ public class SessionRepository {
         remoteSession.deactivate();
     }
 
-    public void delete(RemoteSession remoteSession, Optional<LocalSession> localSession) {
-        localSession.ifPresent(this::deleteLocalSession);
+    public void delete(RemoteSession remoteSession) {
+        LocalSession localSession = getLocalSession(remoteSession.getSessionId());
+        if (localSession != null)
+            deleteLocalSession(localSession);
         remoteSession.deactivate();
     }
 
@@ -603,10 +604,11 @@ public class SessionRepository {
      * Returns a new local session for the given session id if it does not already exist.
      * Will also add the session to the local session cache if necessary
      */
-    public Optional<LocalSession> createLocalSessionUsingDistributedApplicationPackage(long sessionId) {
+    public void createLocalSessionUsingDistributedApplicationPackage(long sessionId) {
         if (applicationRepo.hasLocalSession(sessionId)) {
             log.log(Level.FINE, () -> "Local session for session id " + sessionId + " already exists");
-            return Optional.of(createSessionFromId(sessionId));
+            createSessionFromId(sessionId);
+            return;
         }
 
         SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
@@ -622,16 +624,14 @@ public class SessionRepository {
                 // We cannot be guaranteed that the file reference exists (it could be that it has not
                 // been downloaded yet), and e.g when bootstrapping we cannot throw an exception in that case
                 log.log(Level.INFO, "File reference for session id " + sessionId + ": " + fileReference + " not found in " + fileDirectory);
-                return Optional.empty();
+                return;
             }
             ApplicationId applicationId = sessionZKClient.readApplicationId()
                     .orElseThrow(() -> new RuntimeException("Could not find application id for session " + sessionId));
             log.log(Level.FINE, () -> "Creating local session for tenant '" + tenantName + "' with session id " + sessionId);
             LocalSession localSession = createLocalSession(sessionDir, applicationId, sessionId);
             addLocalSession(localSession);
-            return Optional.of(localSession);
         }
-        return Optional.empty();
     }
 
     private Optional<Long> getActiveSessionId(ApplicationId applicationId) {
@@ -670,15 +670,11 @@ public class SessionRepository {
         return new TenantFileSystemDirs(componentRegistry.getConfigServerDB(), tenantName).getUserApplicationDir(sessionId);
     }
 
-    private void addSessionStateWatcher(long sessionId, RemoteSession remoteSession, Optional<LocalSession> localSession) {
-        // Remote session will always be present in an existing state watcher, but local session might not
-        if (sessionStateWatchers.containsKey(sessionId)) {
-            localSession.ifPresent(session -> sessionStateWatchers.get(sessionId).addLocalSession(session));
-        } else {
+    private void addSessionStateWatcher(long sessionId, RemoteSession remoteSession) {
+        if ( ! sessionStateWatchers.containsKey(sessionId)) {
             Curator.FileCache fileCache = curator.createFileCache(getSessionStatePath(sessionId).getAbsolute(), false);
             fileCache.addListener(this::nodeChanged);
-            sessionStateWatchers.put(sessionId, new SessionStateWatcher(fileCache, remoteSession, localSession,
-                                                                        metrics, zkWatcherExecutor, this));
+            sessionStateWatchers.put(sessionId, new SessionStateWatcher(fileCache, remoteSession, metrics, zkWatcherExecutor, this));
         }
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
index 57d9f027447..c6c08beea17 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
@@ -6,7 +6,6 @@ import com.yahoo.vespa.config.server.monitoring.MetricUpdater;
 import com.yahoo.vespa.curator.Curator;
 import org.apache.curator.framework.recipes.cache.ChildData;
 
-import java.util.Optional;
 import java.util.concurrent.Executor;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -29,17 +28,14 @@ public class SessionStateWatcher {
     private final MetricUpdater metrics;
     private final Executor zkWatcherExecutor;
     private final SessionRepository sessionRepository;
-    private Optional<LocalSession> localSession;
 
     SessionStateWatcher(Curator.FileCache fileCache,
                         RemoteSession remoteSession,
-                        Optional<LocalSession> localSession,
                         MetricUpdater metrics,
                         Executor zkWatcherExecutor,
                         SessionRepository sessionRepository) {
         this.fileCache = fileCache;
         this.remoteSession = remoteSession;
-        this.localSession = localSession;
         this.metrics = metrics;
         this.fileCache.addListener(this::nodeChanged);
         this.fileCache.start();
@@ -65,7 +61,7 @@ public class SessionStateWatcher {
                 sessionRepository.deactivate(remoteSession);
                 break;
             case DELETE:
-                sessionRepository.delete(remoteSession, localSession);
+                sessionRepository.delete(remoteSession);
                 break;
             default:
                 throw new IllegalStateException("Unknown status " + newStatus);
@@ -73,8 +69,8 @@ public class SessionStateWatcher {
     }
 
     private void createLocalSession(long sessionId) {
-        if (sessionRepository.distributeApplicationPackage() && localSession.isEmpty()) {
-            localSession = sessionRepository.createLocalSessionUsingDistributedApplicationPackage(sessionId);
+        if (sessionRepository.distributeApplicationPackage()) {
+            sessionRepository.createLocalSessionUsingDistributedApplicationPackage(sessionId);
         }
     }
 
@@ -109,8 +105,4 @@ public class SessionStateWatcher {
         });
     }
 
-    void addLocalSession(LocalSession session) {
-        localSession = Optional.of(session);
-    }
-
 }