Merge pull request #32827 from vespa-engine/hmusum/simplify-expired-s…

…ession-deletion Simplify expired session deletion
vespa-engine · Nov 11, 2024 · c9f97eb · c9f97eb
2 parents 39ed03d + f77c070
commit c9f97eb
Showing 1 changed file with 22 additions and 34 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
@@ -619,7 +619,8 @@ public void deleteExpiredRemoteAndLocalSessions(Predicate<Session> sessionIsActi
 
         // Avoid deleting too many in one run
         int deleteMax = (int) Math.min(1000, Math.max(50, sessions.size() * 0.05));
-        int deleted = 0;
+        int deletedRemoteSessions = 0;
+        int deletedLocalSessions = 0;
         for (Long sessionId : sessions) {
             try {
                 Session session = remoteSessionCache.get(sessionId);
@@ -638,36 +639,25 @@ public void deleteExpiredRemoteAndLocalSessions(Predicate<Session> sessionIsActi
 
                     log.log(Level.FINE, () -> "Remote session " + sessionId + " for " + tenantName + " has expired, deleting it");
                     deleteRemoteSessionFromZooKeeper(session);
-                    deleted++;
+                    deletedRemoteSessions++;
 
-                    log.log(Level.FINE, () -> "Expired local session is candidate for deletion: " + sessionId +
-                            ", created: " + createTime + ", status " + status + ", can be deleted: " + canBeDeleted(sessionId, status));
-                    // Need to check if it can be deleted, as we might have a (local) session in UNKNOWN
-                    // state that we should not delete
-                    if (canBeDeleted(sessionId, status)) {
+                    var localSessionCanBeDeleted = canBeDeleted(sessionId, status, createTime, activeForApplication);
+                    log.log(Level.FINE, () -> "Expired local session " + sessionId +
+                            ", status " + status + (status == UNKNOWN ? "" : ", created " + createTime) +
+                            ", can be deleted: " + localSessionCanBeDeleted);
+                    if (localSessionCanBeDeleted) {
                         deleteLocalSession(sessionId);
-                        deleted++;
-                        // This might happen if remote session is gone, but local session is not
-                    } else if (isOldAndCanBeDeleted(sessionId, createTime)) {
-                        var localSession = getOptionalSessionFromFileSystem(sessionId);
-                        if (localSession.isEmpty()) continue;
-
-                        // Defensive/paranoid: Check if session is active before deleting
-                        if (! activeForApplication) {
-                            log.log(Level.FINE, "Will delete expired session " + sessionId + " created " +
-                                    createTime + " for '" + applicationId.map(ApplicationId::toString).orElse("unknown") + "'");
-                            deleteLocalSession(sessionId);
-                            deleted++;
-                        }
+                        deletedLocalSessions++;
                     }
-                    if (deleted >= deleteMax)
-                        return;
+                    if (deletedRemoteSessions  + deletedLocalSessions >= deleteMax)
+                        break;
                 }
             } catch (Throwable e) { // Make sure to catch here, to avoid executor just dying in case of issues ...
                 log.log(Level.WARNING, "Error when deleting expired sessions ", e);
             }
         }
-        log.log(Level.FINE, () -> "Done deleting expired sessions");
+        log.log(Level.FINE, "Deleted " + deletedRemoteSessions + " remote and " + deletedLocalSessions +
+                " local sessions that had expired");
     }
 
     private record ApplicationLock(Optional<Lock> lock) implements Closeable {
@@ -691,15 +681,10 @@ private Optional<LocalSession> getOptionalSessionFromFileSystem(long sessionId)
         return Optional.empty();
     }
 
-    private boolean isOldAndCanBeDeleted(long sessionId, Instant createTime) {
+    private boolean isOldAndCanBeDeleted(Instant createTime) {
         Duration oneDay = Duration.ofDays(1);
         Duration expiry = Duration.ofSeconds(Math.max(sessionLifeTimeInSeconds(), oneDay.getSeconds()));
-        if (createTime.plus(expiry).isBefore(clock.instant())) {
-            log.log(Level.FINE, () -> "more than 1 day old: " + sessionId);
-            return true;
-        } else {
-            return false;
-        }
+        return createTime.plus(expiry).isBefore(clock.instant());
     }
 
     private boolean hasExpired(Instant created) {
@@ -710,10 +695,13 @@ private boolean hasExpired(Instant created) {
 
     private long sessionLifeTimeInSeconds() { return configserverConfig.sessionLifetime(); }
 
-    // Sessions with state other than UNKNOWN or ACTIVATE or old sessions in UNKNOWN state
-    private boolean canBeDeleted(long sessionId, Session.Status status) {
-        return ( ! List.of(UNKNOWN, ACTIVATE).contains(status))
-                || oldSessionDirWithUnknownStatus(sessionId, status);
+    private boolean canBeDeleted(long sessionId, Session.Status status, Instant createTime, boolean activeForApplication) {
+        // Delete Sessions with state other than UNKNOWN or ACTIVATE or old sessions in UNKNOWN state
+        if ( ! List.of(UNKNOWN, ACTIVATE).contains(status) || oldSessionDirWithUnknownStatus(sessionId, status))
+            return true;
+
+        // This might happen if remote session is gone, but local session is not
+        return isOldAndCanBeDeleted(createTime) && !activeForApplication;
     }
 
     private boolean oldSessionDirWithUnknownStatus(long sessionId, Session.Status status) {