Merge pull request #28235 from vespa-engine/hmusum/fix-read-active-session

Read session data in just one place and always fallback to reading ol…

4 files changed, 34 insertions, 42 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
index 70b8dc5f51a..38be13894e2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabase.java
@@ -136,10 +136,7 @@ public class ApplicationCuratorDatabase {
      * Returns Optional.empty() if application not found or no active session exists.
      */
     public Optional<Long> activeSessionOf(ApplicationId id) {
-        Optional<byte[]> data = curator.getData(applicationPath(id));
-        return (data.isEmpty() || data.get().length == 0)
-               ? Optional.empty()
-               : data.map(bytes -> Long.parseLong(Utf8.toString(bytes)));
+        return applicationData(id).flatMap(ApplicationData::activeSession);
     }
 
     /**
@@ -147,25 +144,13 @@ public class ApplicationCuratorDatabase {
      * Returns Optional.empty() if application not found or no application data exists.
      */
     public Optional<ApplicationData> applicationData(ApplicationId id) {
-        return applicationData(id, false);
-    }
-
-    /**
-     * Returns application data for the given application.
-     * Returns Optional.empty() if application not found or no application data exists.
-     */
-    public Optional<ApplicationData> applicationData(ApplicationId id, boolean readAsJson) {
         Optional<byte[]> data = curator.getData(applicationPath(id));
         if (data.isEmpty() || data.get().length == 0) return Optional.empty();
 
-        if (readAsJson) {
-            try {
-                return Optional.of(ApplicationData.fromBytes(data.get()));
-            } catch (IllegalArgumentException e) {
-                return applicationDataOldFormat(id, readAsJson);
-            }
-        } else {
-            return applicationDataOldFormat(id, readAsJson);
+        try {
+            return Optional.of(ApplicationData.fromBytes(data.get()));
+        } catch (IllegalArgumentException e) {
+            return applicationDataOldFormat(id);
         }
     }
 
@@ -173,7 +158,7 @@ public class ApplicationCuratorDatabase {
      * Returns application data for the given application.
      * Returns Optional.empty() if application not found or no application data exists.
      */
-    public Optional<ApplicationData> applicationDataOldFormat(ApplicationId id, boolean readAsJson) {
+    public Optional<ApplicationData> applicationDataOldFormat(ApplicationId id) {
         Optional<byte[]> data = curator.getData(applicationPath(id));
         if (data.isEmpty() || data.get().length == 0) return Optional.empty();
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationData.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationData.java
index 868ea060fba..31e54bd67a0 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationData.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationData.java
@@ -6,6 +6,7 @@ import com.yahoo.slime.Slime;
 import com.yahoo.slime.SlimeUtils;
 
 import java.io.IOException;
+import java.util.Optional;
 import java.util.OptionalLong;
 
 import static com.yahoo.slime.SlimeUtils.optionalLong;
@@ -54,9 +55,17 @@ public class ApplicationData {
 
     public ApplicationId applicationId() { return applicationId; }
 
-    public OptionalLong activeSession() { return activeSession; }
+    public Optional<Long> activeSession() {
+        return Optional.of(activeSession)
+            .filter(OptionalLong::isPresent)
+            .map(OptionalLong::getAsLong);
+    }
 
-    public OptionalLong lastDeployedSession() { return lastDeployedSession; }
+    public Optional<Long> lastDeployedSession() {
+        return Optional.of(lastDeployedSession)
+                .filter(OptionalLong::isPresent)
+                .map(OptionalLong::getAsLong);
+    }
 
     @Override
     public String toString() {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
index 538534d0040..1d1ed1042ee 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
@@ -80,7 +80,6 @@ public class TenantApplications implements RequestHandler, HostValidator {
     private final String serverId;
     private final ListFlag<String> incompatibleVersions;
     private final BooleanFlag writeApplicationDataAsJson;
-    private final BooleanFlag readApplicationDataAsJson;
 
     public TenantApplications(TenantName tenant, Curator curator, StripedExecutor<TenantName> zkWatcherExecutor,
                               ExecutorService zkCacheExecutor, Metrics metrics, ConfigActivationListener configActivationListener,
@@ -103,7 +102,6 @@ public class TenantApplications implements RequestHandler, HostValidator {
         this.serverId = configserverConfig.serverId();
         this.incompatibleVersions = PermanentFlags.INCOMPATIBLE_VERSIONS.bindTo(flagSource);
         this.writeApplicationDataAsJson = Flags.WRITE_APPLICATION_DATA_AS_JSON.bindTo(flagSource);
-        this.readApplicationDataAsJson = Flags.READ_APPLICATION_DATA_AS_JSON.bindTo(flagSource);
     }
 
     /** The curator backed ZK storage of this. */
@@ -135,7 +133,7 @@ public class TenantApplications implements RequestHandler, HostValidator {
      * Returns Optional.empty if application not found or no application data exists.
      */
     public Optional<ApplicationData> applicationData(ApplicationId id) {
-        return database().applicationData(id, readApplicationDataAsJson.value());
+        return database().applicationData(id);
     }
 
     public boolean sessionExistsInFileSystem(long sessionId) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
index 8d1b22c94c5..80f705dcbad 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationCuratorDatabaseTest.java
@@ -51,15 +51,8 @@ public class ApplicationCuratorDatabaseTest {
         assertEquals(Optional.empty(), db.applicationData(id)); // still empty, as no data has been written to node
 
         db.createApplication(id, true);
-        try {
-            Optional<ApplicationData> applicationData = db.applicationData(id);
-            fail("Expected exception, got " + applicationData);
-        } catch (NumberFormatException e) {
-            // expected
-        }
-
         // Can be read as json, but no active session or last deployed session
-        Optional<ApplicationData> applicationData = db.applicationData(id, true);
+        Optional<ApplicationData> applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
         assertEquals(id, applicationData.get().applicationId());
         assertFalse(applicationData.get().activeSession().isPresent());
@@ -74,11 +67,18 @@ public class ApplicationCuratorDatabaseTest {
             t.commit();
         }
         // Can be read as session id only
-        applicationData = db.applicationData(id, false);
+        applicationData = db.applicationData(id);
+        assertTrue(applicationData.isPresent());
+        assertEquals(id, applicationData.get().applicationId());
+        assertTrue(applicationData.get().activeSession().isPresent());
+        assertEquals(2, applicationData.get().activeSession().get().longValue());
+        assertFalse(applicationData.get().lastDeployedSession().isPresent());
+        // Can be read as session data as well
+        applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
         assertEquals(id, applicationData.get().applicationId());
         assertTrue(applicationData.get().activeSession().isPresent());
-        assertEquals(2, applicationData.get().activeSession().getAsLong());
+        assertEquals(2, applicationData.get().activeSession().get().longValue());
         assertFalse(applicationData.get().lastDeployedSession().isPresent());
 
         // Prepare session 3, last deployed session is still 2
@@ -86,25 +86,25 @@ public class ApplicationCuratorDatabaseTest {
             t.commit();
         }
         // Can be read as json, active session is still 2 and last deployed session is 3
-        applicationData = db.applicationData(id, true);
+        applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
         assertEquals(id, applicationData.get().applicationId());
         assertTrue(applicationData.get().activeSession().isPresent());
-        assertEquals(2, applicationData.get().activeSession().getAsLong());
+        assertEquals(2L, applicationData.get().activeSession().get().longValue());
         assertTrue(applicationData.get().lastDeployedSession().isPresent());
-        assertEquals(3, applicationData.get().lastDeployedSession().getAsLong());
+        assertEquals(3, applicationData.get().lastDeployedSession().get().longValue());
 
         try (var t = db.createWriteActiveTransaction(new CuratorTransaction(curator), id, 3, true)) {
             t.commit();
         }
         // Can be read as json, active session and last deployed session present
-        applicationData = db.applicationData(id, true);
+        applicationData = db.applicationData(id);
         assertTrue(applicationData.isPresent());
         assertEquals(id, applicationData.get().applicationId());
         assertTrue(applicationData.get().activeSession().isPresent());
-        assertEquals(3, applicationData.get().activeSession().getAsLong());
+        assertEquals(3, applicationData.get().activeSession().get().longValue());
         assertTrue(applicationData.get().lastDeployedSession().isPresent());
-        assertEquals(3, applicationData.get().lastDeployedSession().getAsLong());
+        assertEquals(3, applicationData.get().lastDeployedSession().get().longValue());
     }
 
 }