Move the cache part of SessionRepo into its own class

7 files changed, 57 insertions, 36 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 490ab988048..dff70e6481e 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
@@ -678,7 +678,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     public void deleteExpiredLocalSessions() {
         Map<Tenant, List<LocalSession>> sessionsPerTenant = new HashMap<>();
-        tenantRepository.getAllTenants().forEach(tenant -> sessionsPerTenant.put(tenant, tenant.getLocalSessionRepo().listSessions()));
+        tenantRepository.getAllTenants().forEach(tenant -> sessionsPerTenant.put(tenant, tenant.getLocalSessionRepo().getSessions()));
 
         Set<ApplicationId> applicationIds = new HashSet<>();
         sessionsPerTenant.values().forEach(sessionList -> sessionList.forEach(s -> applicationIds.add(s.getApplicationId())));
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java
index d276a59327d..3347ba4a63e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java
@@ -28,15 +28,17 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
- * File-based session repository for LocalSessions. Contains state for the local instance of the configserver.
+ *
+ * Contains state for the local instance of the configserver.
  *
  * @author Ulf Lilleengen
  */
-public class LocalSessionRepo extends SessionRepo<LocalSession> {
+public class LocalSessionRepo {
 
     private static final Logger log = Logger.getLogger(LocalSessionRepo.class.getName());
     private static final FilenameFilter sessionApplicationsFilter = (dir, name) -> name.matches("\\d+");
 
+    private final SessionCache<LocalSession> sessionCache;
     private final Map<Long, LocalSessionStateWatcher> sessionStateWatchers = new HashMap<>();
     private final long sessionLifetime; // in seconds
     private final Clock clock;
@@ -52,6 +54,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> {
 
     // Constructor public only for testing
     public LocalSessionRepo(TenantName tenantName, GlobalComponentRegistry componentRegistry) {
+        sessionCache = new SessionCache<>();
         this.clock = componentRegistry.getClock();
         this.curator = componentRegistry.getCurator();
         this.sessionLifetime = componentRegistry.getConfigserverConfig().sessionLifetime();
@@ -60,15 +63,22 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> {
         this.expiryTimeFlag = Flags.CONFIGSERVER_LOCAL_SESSIONS_EXPIRY_INTERVAL_IN_DAYS.bindTo(componentRegistry.getFlagSource());
     }
 
-    @Override
     public synchronized void addSession(LocalSession session) {
-        super.addSession(session);
+        sessionCache.addSession(session);
         Path sessionsPath = TenantRepository.getSessionsPath(session.getTenantName());
         long sessionId = session.getSessionId();
         Curator.FileCache fileCache = curator.createFileCache(sessionsPath.append(String.valueOf(sessionId)).append(ConfigCurator.SESSIONSTATE_ZK_SUBPATH).getAbsolute(), false);
         sessionStateWatchers.put(sessionId, new LocalSessionStateWatcher(fileCache, session, this, zkWatcherExecutor));
     }
 
+    public LocalSession getSession(long sessionId) {
+        return sessionCache.getSession(sessionId);
+    }
+
+    public List<LocalSession> getSessions() {
+        return sessionCache.getSessions();
+    }
+
     private void loadSessions(LocalSessionLoader loader) {
         File[] sessions = tenantFileSystemDirs.sessionsPath().listFiles(sessionApplicationsFilter);
         if (sessions == null) {
@@ -87,7 +97,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> {
     public void deleteExpiredSessions(Map<ApplicationId, Long> activeSessions) {
         log.log(Level.FINE, "Purging old sessions");
         try {
-            for (LocalSession candidate : listSessions()) {
+            for (LocalSession candidate : sessionCache.getSessions()) {
                 Instant createTime = Instant.ofEpochSecond(candidate.getCreateTime());
                 log.log(Level.FINE, "Candidate session for deletion: " + candidate.getSessionId() + ", created: " + createTime);
 
@@ -125,7 +135,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> {
         log.log(Level.FINE, "Deleting local session " + sessionId);
         LocalSessionStateWatcher watcher = sessionStateWatchers.remove(sessionId);
         if (watcher != null)  watcher.close();
-        removeSession(sessionId);
+        sessionCache.removeSession(sessionId);
         NestedTransaction transaction = new NestedTransaction();
         session.delete(transaction);
         transaction.commit();
@@ -137,7 +147,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> {
     }
 
     private void deleteAllSessions() {
-        List<LocalSession> sessions = new ArrayList<>(listSessions());
+        List<LocalSession> sessions = new ArrayList<>(sessionCache.getSessions());
         for (LocalSession session : sessions) {
             deleteSession(session);
         }
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 c27b7c6802b..0189ad42170 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
@@ -30,11 +30,11 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
+ * Session repository for RemoteSessions. There is one such repo per tenant.
  * Will watch/prepare sessions (applications) based on watched nodes in ZooKeeper. The zookeeper state watched in
  * this class is shared between all config servers, so it should not modify any global state, because the operation
  * will be performed on all servers. The repo can be regarded as read only from the POV of the configserver.
@@ -42,7 +42,7 @@ import java.util.stream.Collectors;
  * @author Vegard Havdal
  * @author Ulf Lilleengen
  */
-public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
+public class RemoteSessionRepo {
 
     private static final Logger log = Logger.getLogger(RemoteSessionRepo.class.getName());
 
@@ -57,12 +57,14 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
     private final Curator.DirectoryCache directoryCache;
     private final TenantApplications applicationRepo;
     private final Executor zkWatcherExecutor;
+    private final SessionCache<RemoteSession> sessionCache;
 
     public RemoteSessionRepo(GlobalComponentRegistry componentRegistry,
                              RemoteSessionFactory remoteSessionFactory,
                              ReloadHandler reloadHandler,
                              TenantName tenantName,
                              TenantApplications applicationRepo) {
+        this.sessionCache = new SessionCache<>();
         this.componentRegistry = componentRegistry;
         this.curator = componentRegistry.getCurator();
         this.sessionsPath = TenantRepository.getSessionsPath(tenantName);
@@ -79,14 +81,22 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
         this.directoryCache.start();
     }
 
+    public RemoteSession getSession(long sessionId) {
+        return sessionCache.getSession(sessionId);
+    }
+
     public List<Long> getSessions() {
         return getSessionList(curator.getChildren(sessionsPath));
     }
 
+    public void addSession(RemoteSession session) {
+        sessionCache.addSession(session);
+    }
+
     public int deleteExpiredSessions(Clock clock, Duration expiryTime) {
         int deleted = 0;
         for (long sessionId : getSessions()) {
-            RemoteSession session = getSession(sessionId);
+            RemoteSession session = sessionCache.getSession(sessionId);
             if (session == null) continue; // Internal sessions not in synch with zk, continue
             if (session.getStatus() == Session.Status.ACTIVATE) continue;
             Instant created = Instant.ofEpochSecond(session.getCreateTime());
@@ -124,14 +134,14 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
     }
 
     private void checkForRemovedSessions(List<Long> sessions) {
-        for (RemoteSession session : listSessions())
+        for (RemoteSession session : sessionCache.getSessions())
             if ( ! sessions.contains(session.getSessionId()))
                 sessionRemoved(session.getSessionId());
     }
     
     private void checkForAddedSessions(List<Long> sessions) {
         for (Long sessionId : sessions)
-            if (getSession(sessionId) == null)
+            if (sessionCache.getSession(sessionId) == null)
                 sessionAdded(sessionId);
     }
 
@@ -148,7 +158,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
             Curator.FileCache fileCache = curator.createFileCache(sessionPath.append(ConfigCurator.SESSIONSTATE_ZK_SUBPATH).getAbsolute(), false);
             fileCache.addListener(this::nodeChanged);
             loadSessionIfActive(session);
-            addSession(session);
+            sessionCache.addSession(session);
             metrics.incAddedSessions();
             sessionStateWatchers.put(sessionId, new RemoteSessionStateWatcher(fileCache, reloadHandler, session, metrics, zkWatcherExecutor));
         } catch (Exception e) {
@@ -160,7 +170,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
     private void sessionRemoved(long sessionId) {
         RemoteSessionStateWatcher watcher = sessionStateWatchers.remove(sessionId);
         if (watcher != null)  watcher.close();
-        removeSession(sessionId);
+        sessionCache.removeSession(sessionId);
         metrics.incRemovedSessions();
     }
 
@@ -190,7 +200,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
     private void nodeChanged() {
         zkWatcherExecutor.execute(() -> {
             Multiset<Session.Status> sessionMetrics = HashMultiset.create();
-            for (RemoteSession session : listSessions()) {
+            for (RemoteSession session : sessionCache.getSessions()) {
                 sessionMetrics.add(session.getStatus());
             }
             metrics.setNewSessions(sessionMetrics.count(Session.Status.NEW));
@@ -221,7 +231,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> {
 
     private void synchronizeOnNew(List<Long> sessionList) {
         for (long sessionId : sessionList) {
-            RemoteSession session = getSession(sessionId);
+            RemoteSession session = sessionCache.getSession(sessionId);
             if (session == null) continue; // session might have been deleted after getting session list
             log.log(Level.FINE, () -> session.logPre() + "Confirming upload for session " + sessionId);
             session.confirmUpload();
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/SessionCache.java
index 0cc166dc6e3..83ea342e5da 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/SessionCache.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.session;
 
 import java.util.ArrayList;
@@ -6,12 +6,12 @@ import java.util.HashMap;
 import java.util.List;
 
 /**
- * A generic session repository that can store any type of session that extends the abstract interface.
+ * A session cache that can store any type of session that extends the abstract interface.
  *
  * @author Ulf Lilleengen
+ * @author hmusum
  */
-// TODO: This is a ZK cache. We should probably remove it, or make that explicit
-public class SessionRepo<SESSIONTYPE extends Session> {
+public class SessionCache<SESSIONTYPE extends Session> {
 
     private final HashMap<Long, SESSIONTYPE> sessions = new HashMap<>();
 
@@ -37,7 +37,7 @@ public class SessionRepo<SESSIONTYPE extends Session> {
         return sessions.get(id);
     }
 
-    public synchronized List<SESSIONTYPE> listSessions() {
+    public synchronized List<SESSIONTYPE> getSessions() {
         return new ArrayList<>(sessions.values());
     }
     
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
index 27705e9c1e0..f26fa05baf8 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
@@ -333,13 +333,13 @@ public class ApplicationRepositoryTest {
         // No change to active session id
         assertEquals(activeSessionId, tester.tenant().getApplicationRepo().requireActiveSessionOf(tester.applicationId()));
         LocalSessionRepo localSessionRepo = tester.tenant().getLocalSessionRepo();
-        assertEquals(3, localSessionRepo.listSessions().size());
+        assertEquals(3, localSessionRepo.getSessions().size());
 
         clock.advance(Duration.ofHours(1)); // longer than session lifetime
 
         // All sessions except 3 should be removed after the call to deleteExpiredLocalSessions
         tester.applicationRepository().deleteExpiredLocalSessions();
-        Collection<LocalSession> sessions = localSessionRepo.listSessions();
+        Collection<LocalSession> sessions = localSessionRepo.getSessions();
         assertEquals(1, sessions.size());
         ArrayList<LocalSession> localSessions = new ArrayList<>(sessions);
         LocalSession localSession = localSessions.get(0);
@@ -353,9 +353,9 @@ public class ApplicationRepositoryTest {
         assertTrue(deployment4.isPresent());
         deployment4.get().prepare();  // session 5 (not activated)
 
-        assertEquals(2, localSessionRepo.listSessions().size());
+        assertEquals(2, localSessionRepo.getSessions().size());
         localSessionRepo.deleteSession(localSession);
-        assertEquals(1, localSessionRepo.listSessions().size());
+        assertEquals(1, localSessionRepo.getSessions().size());
 
         // Check that trying to expire when there are no active sessions works
         tester.applicationRepository().deleteExpiredLocalSessions();
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java
index 15ebe425e45..ecff6bec979 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java
@@ -103,9 +103,9 @@ public class RemoteSessionRepoTest {
                                      .build();
         curator.create(TenantRepository.getSessionsPath(mytenant));
         remoteSessionRepo = tenant.getRemoteSessionRepo();
-        assertThat(remoteSessionRepo.listSessions().size(), is(0));
+        assertThat(remoteSessionRepo.getSessions().size(), is(0));
         createSession(sessionId, true, mytenant);
-        assertThat(remoteSessionRepo.listSessions().size(), is(1));
+        assertThat(remoteSessionRepo.getSessions().size(), is(1));
     }
 
     private void assertStatusChange(long sessionId, Session.Status status) throws Exception {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepoTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionCacheTest.java
index b8a3d0bc401..15d315bc00a 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepoTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionCacheTest.java
@@ -13,21 +13,22 @@ import static org.junit.Assert.assertThat;
 /**
  * @author hmusum
  */
-public class SessionRepoTest {
+public class SessionCacheTest {
+
     @Test
     public void require_that_sessionrepo_is_initialized() {
-        SessionRepo<TestSession> sessionRepo = new SessionRepo<>();
-        assertNull(sessionRepo.getSession(1L));
-        sessionRepo.addSession(new TestSession(1));
-        assertThat(sessionRepo.getSession(1L).getSessionId(), is(1L));
+        SessionCache<TestSession> sessionCache = new SessionCache<>();
+        assertNull(sessionCache.getSession(1L));
+        sessionCache.addSession(new TestSession(1));
+        assertThat(sessionCache.getSession(1L).getSessionId(), is(1L));
     }
 
     @Test(expected = IllegalArgumentException.class)
     public void require_that_adding_existing_session_fails() {
-        SessionRepo<TestSession> sessionRepo = new SessionRepo<>();
+        SessionCache<TestSession> sessionCache = new SessionCache<>();
         final TestSession session = new TestSession(1);
-        sessionRepo.addSession(session);
-        sessionRepo.addSession(session);
+        sessionCache.addSession(session);
+        sessionCache.addSession(session);
     }
 
     private class TestSession extends Session {