Merge pull request #15164 from vespa-engine/jonmv/reindexing-data-take-2

Lazy config gen checking and set ready to now if previous value is old

6 files changed, 64 insertions, 27 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 4bc3066ee18..8c3c9882aa1 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
@@ -1,7 +1,6 @@
 // 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.application;
 
-import com.yahoo.concurrent.StripedExecutor;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.path.Path;
@@ -19,7 +18,6 @@ import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.curator.transaction.CuratorOperations;
 import com.yahoo.vespa.curator.transaction.CuratorTransaction;
 import com.yahoo.yolean.Exceptions;
-import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
 
 import java.time.Duration;
 import java.time.Instant;
@@ -116,10 +114,9 @@ public class ApplicationCuratorDatabase {
                       .collect(Collectors.toUnmodifiableList());
     }
 
-    public ApplicationReindexing readReindexingStatus(ApplicationId id) {
+    public Optional<ApplicationReindexing> readReindexingStatus(ApplicationId id) {
         return curator.getData(reindexingDataPath(id))
-                      .map(data -> ReindexingStatusSerializer.fromBytes(data))
-                      .orElse(ApplicationReindexing.empty());
+                      .map(data -> ReindexingStatusSerializer.fromBytes(data));
     }
 
     public void writeReindexingStatus(ApplicationId id, ApplicationReindexing status) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
index 918aee73874..eef65d969fc 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
@@ -21,8 +21,6 @@ import static java.util.stream.Collectors.toUnmodifiableMap;
  */
 public class ApplicationReindexing implements Reindexing {
 
-    private static final ApplicationReindexing empty = new ApplicationReindexing(Status.ALWAYS_READY, Map.of());
-
     private final Status common;
     private final Map<String, Cluster> clusters;
 
@@ -31,9 +29,9 @@ public class ApplicationReindexing implements Reindexing {
         this.clusters = Map.copyOf(clusters);
     }
 
-    /** No reindexing pending or ready. */
-    public static ApplicationReindexing empty() {
-        return empty;
+    /** Reindexing for the whole application ready now. */
+    public static ApplicationReindexing ready(Instant now) {
+        return new ApplicationReindexing(new Status(now), Map.of());
     }
 
     /** Returns a copy of this with common reindexing for the whole application ready at the given instant. */
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainer.java
index cf66d8d5b81..a301b091a0f 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainer.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.config.server.maintenance;
 
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.vespa.config.server.ApplicationRepository;
+import com.yahoo.vespa.config.server.application.Application;
 import com.yahoo.vespa.config.server.application.ApplicationCuratorDatabase;
 import com.yahoo.vespa.config.server.application.ApplicationReindexing;
 import com.yahoo.vespa.config.server.application.ConfigConvergenceChecker;
@@ -19,6 +20,9 @@ import java.util.Collection;
 import java.util.Comparator;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -34,6 +38,8 @@ public class ReindexingMaintainer extends ConfigServerMaintainer {
     /** Timeout per service when getting config generations. */
     private static final Duration timeout = Duration.ofSeconds(10);
 
+    static final Duration reindexingInterval = Duration.ofDays(30);
+
     private final ConfigConvergenceChecker convergence;
     private final Clock clock;
 
@@ -56,8 +62,9 @@ public class ReindexingMaintainer extends ConfigServerMaintainer {
                                          try {
                                              Collection<Long> generations = convergence.getServiceConfigGenerations(application, timeout).values();
                                              try (Lock lock = database.lock(id)) {
-                                                 ApplicationReindexing reindexing = database.readReindexingStatus(id);
-                                                 database.writeReindexingStatus(id, withReady(reindexing, generations, clock.instant()));
+                                                 ApplicationReindexing reindexing = database.readReindexingStatus(id)
+                                                                                            .orElse(ApplicationReindexing.ready(clock.instant()));
+                                                 database.writeReindexingStatus(id, withReady(reindexing, lazyGeneration(application), clock.instant()));
                                              }
                                          }
                                          catch (RuntimeException e) {
@@ -69,13 +76,34 @@ public class ReindexingMaintainer extends ConfigServerMaintainer {
         return success.get();
     }
 
-    static ApplicationReindexing withReady(ApplicationReindexing reindexing, Collection<Long> generations, Instant now) {
-        long oldestGeneration = generations.stream().min(Comparator.naturalOrder()).orElse(-1L);
-        for (var cluster : reindexing.clusters().entrySet())
+    private Supplier<Long> lazyGeneration(Application application) {
+        AtomicLong oldest = new AtomicLong();
+        return () -> {
+            if (oldest.get() == 0)
+                oldest.set(convergence.getServiceConfigGenerations(application, timeout).values().stream()
+                                      .min(Comparator.naturalOrder())
+                                      .orElse(-1L));
+
+            return oldest.get();
+        };
+    }
+
+    static ApplicationReindexing withReady(ApplicationReindexing reindexing, Supplier<Long> oldestGeneration, Instant now) {
+        for (var cluster : reindexing.clusters().entrySet()) {
             for (var pending : cluster.getValue().pending().entrySet())
-                if (pending.getValue() <= oldestGeneration)
+                if (pending.getValue() <= oldestGeneration.get())
                     reindexing = reindexing.withReady(cluster.getKey(), pending.getKey(), now);
 
+            for (var documentType : cluster.getValue().ready().entrySet())
+                if (documentType.getValue().ready().isBefore(now.minus(reindexingInterval)))
+                    reindexing = reindexing.withReady(cluster.getKey(), documentType.getKey(), now);
+
+            if (cluster.getValue().common().ready().isBefore(now.minus(reindexingInterval)))
+                reindexing = reindexing.withReady(cluster.getKey(), now);
+        }
+        if (reindexing.common().ready().isBefore(now.minus(reindexingInterval)))
+            reindexing = reindexing.withReady(now);
+
         return reindexing;
     }
 
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 89ee1dcccce..b74df3ffd9a 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
@@ -6,6 +6,7 @@ import com.yahoo.vespa.curator.mock.MockCurator;
 import org.junit.Test;
 
 import java.time.Instant;
+import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 
@@ -19,9 +20,9 @@ public class ApplicationCuratorDatabaseTest {
         ApplicationId id = ApplicationId.defaultId();
         ApplicationCuratorDatabase db = new ApplicationCuratorDatabase(id.tenant(), new MockCurator());
 
-        assertEquals(ApplicationReindexing.empty(), db.readReindexingStatus(id));
+        assertEquals(Optional.empty(), db.readReindexingStatus(id));
 
-        ApplicationReindexing reindexing = ApplicationReindexing.empty()
+        ApplicationReindexing reindexing = ApplicationReindexing.ready(Instant.EPOCH)
                                                                 .withReady(Instant.ofEpochMilli(1 << 20))
                                                                 .withPending("one", "a", 10)
                                                                 .withReady("two", "b", Instant.ofEpochMilli(2))
@@ -31,7 +32,7 @@ public class ApplicationCuratorDatabaseTest {
                                                                 .withReady("two", "c", Instant.ofEpochMilli(3));
 
         db.writeReindexingStatus(id, reindexing);
-        assertEquals(reindexing, db.readReindexingStatus(id));
+        assertEquals(reindexing, db.readReindexingStatus(id).orElseThrow());
     }
 
 }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
index 1b0bad95acc..71661776095 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
@@ -17,7 +17,7 @@ public class ApplicationReindexingTest {
 
     @Test
     public void test() {
-        ApplicationReindexing reindexing = ApplicationReindexing.empty()
+        ApplicationReindexing reindexing = ApplicationReindexing.ready(Instant.EPOCH)
                                                                 .withReady(Instant.ofEpochMilli(1 << 20))
                                                                 .withPending("one", "a", 10)
                                                                 .withReady("two", "b", Instant.ofEpochMilli(2))
@@ -26,6 +26,15 @@ public class ApplicationReindexingTest {
                                                                 .withReady("one", "a", Instant.ofEpochMilli(1))
                                                                 .withReady("two", "c", Instant.ofEpochMilli(3));
 
+        assertEquals(Instant.ofEpochMilli(1 << 20),
+                     reindexing.status("one", "a").ready());
+
+        assertEquals(Instant.ofEpochMilli(1 << 20),
+                     reindexing.status("one", "d").ready());
+
+        assertEquals(Instant.ofEpochMilli(1 << 20),
+                     reindexing.status("three", "a").ready());
+
         assertEquals(new Status(Instant.ofEpochMilli(1 << 20)),
                      reindexing.common());
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainerTest.java
index 93077e0b941..f4a553e25b7 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/maintenance/ReindexingMaintainerTest.java
@@ -5,7 +5,6 @@ import com.yahoo.vespa.config.server.application.ApplicationReindexing;
 import org.junit.Test;
 
 import java.time.Instant;
-import java.util.List;
 
 import static com.yahoo.vespa.config.server.maintenance.ReindexingMaintainer.withReady;
 import static org.junit.Assert.assertEquals;
@@ -17,8 +16,7 @@ public class ReindexingMaintainerTest {
 
     @Test
     public void testReadyComputation() {
-        ApplicationReindexing reindexing = ApplicationReindexing.empty()
-                                                                .withReady(Instant.ofEpochMilli(1 << 20))
+        ApplicationReindexing reindexing = ApplicationReindexing.ready(Instant.ofEpochMilli(1 << 20))
                                                                 .withPending("one", "a", 10)
                                                                 .withReady("two", "b", Instant.ofEpochMilli(2))
                                                                 .withPending("two", "b", 20)
@@ -27,13 +25,19 @@ public class ReindexingMaintainerTest {
                                                                 .withReady("two", "c", Instant.ofEpochMilli(3));
 
         assertEquals(reindexing,
-                     withReady(reindexing, List.of(), Instant.EPOCH));
+                     withReady(reindexing, () -> -1L, Instant.EPOCH));
 
         assertEquals(reindexing,
-                     withReady(reindexing, List.of(19L), Instant.EPOCH));
+                     withReady(reindexing, () -> 19L, Instant.EPOCH));
 
-        assertEquals(reindexing.withReady("two", "b", Instant.MAX),
-                     withReady(reindexing, List.of(20L), Instant.MAX));
+        Instant later = Instant.ofEpochMilli(2).plus(ReindexingMaintainer.reindexingInterval);
+        assertEquals(reindexing.withReady("one", later)         // Had EPOCH as previous, so is updated.
+                               .withReady("two", "b", later)    // Got config convergence, so is updated.
+                               .withReady("one", "a", later),   // Had EPOCH + 1 as previous, so is updated.
+                     withReady(reindexing, () -> 20L, later));
+
+        // Verify generation supplier isn't called when no pending document types.
+        withReady(reindexing.withReady("two", "b", later), () -> { throw new AssertionError("not supposed to run"); }, later);
     }
 
 }