Merge pull request #15216 from vespa-engine/jonmv/use-most-specific-reindexing-ready-timestamp

Use most specific, not latest, reindexing ready timestamp, in config

3 files changed, 19 insertions, 17 deletions

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 fae52f318bd..ed28bec0d81 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
@@ -42,14 +42,14 @@ public class ApplicationReindexing implements Reindexing {
 
     /** Returns a copy of this with common reindexing for the given cluster ready at the given instant. */
     public ApplicationReindexing withReady(String cluster, Instant readyAt) {
-        Cluster current = clusters.getOrDefault(cluster, Cluster.empty);
+        Cluster current = clusters.getOrDefault(cluster, Cluster.ready(common));
         Cluster modified = new Cluster(new Status(readyAt), current.pending, current.ready);
         return new ApplicationReindexing(common, with(cluster, modified, clusters));
     }
 
     /** Returns a copy of this with reindexing for the given document type in the given cluster ready at the given instant. */
     public ApplicationReindexing withReady(String cluster, String documentType, Instant readyAt) {
-        Cluster current = clusters.getOrDefault(cluster, Cluster.empty);
+        Cluster current = clusters.getOrDefault(cluster, Cluster.ready(common));
         Cluster modified = new Cluster(current.common,
                                        without(documentType, current.pending),
                                        with(documentType, new Status(readyAt), current.ready));
@@ -58,7 +58,7 @@ public class ApplicationReindexing implements Reindexing {
 
     /** Returns a copy of this with a pending reindexing at the given generation, for the given document type. */
     public ApplicationReindexing withPending(String cluster, String documentType, long requiredGeneration) {
-        Cluster current = clusters.getOrDefault(cluster, Cluster.empty);
+        Cluster current = clusters.getOrDefault(cluster, Cluster.ready(common));
         Cluster modified = new Cluster(current.common,
                                        with(documentType, requirePositive(requiredGeneration), current.pending),
                                        without(documentType, current.ready));
@@ -79,13 +79,10 @@ public class ApplicationReindexing implements Reindexing {
             if (clusters.get(cluster).pending().containsKey(documentType))
                 return Optional.empty();
 
-            Status documentStatus = clusters.get(cluster).ready().get(documentType);
-            Status clusterStatus = clusters.get(cluster).common();
-            if (documentStatus == null || documentStatus.ready().isBefore(clusterStatus.ready()))
-                documentStatus = clusterStatus;
+            if (clusters.get(cluster).ready().containsKey(documentType))
+                return Optional.of(clusters.get(cluster).ready().get(documentType));
 
-            if (documentStatus.ready().isAfter(common().ready()))
-                return Optional.of(documentStatus);
+            return Optional.of(clusters.get(cluster).common());
         }
         return Optional.of(common());
     }
@@ -116,7 +113,7 @@ public class ApplicationReindexing implements Reindexing {
     /** Reindexing status for a single content cluster in an application. */
     public static class Cluster {
 
-        private static final Cluster empty = new Cluster(Status.ALWAYS_READY, Map.of(), Map.of());
+        private static Cluster ready(Status common) { return new Cluster(common, Map.of(), Map.of()); }
 
         private final Status common;
         private final Map<String, Long> pending;
@@ -173,9 +170,6 @@ public class ApplicationReindexing implements Reindexing {
     /** Reindexing status common to an application, one of its clusters, or a single document type in a cluster. */
     public static class Status implements Reindexing.Status {
 
-        /** Always ready, i.e., ignored when joining with more specific statuses. */
-        private static final Status ALWAYS_READY = new Status(Instant.EPOCH);
-
         private final Instant ready;
 
         Status(Instant ready) {
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 b803413d6ea..82de3e2eefb 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
@@ -27,12 +27,19 @@ public class ApplicationReindexingTest {
                                                                 .withReady("one", "a", Instant.ofEpochMilli(1))
                                                                 .withReady("two", "c", Instant.ofEpochMilli(3));
 
-        assertEquals(Instant.ofEpochMilli(1 << 20),
+        // Document is most specific, and is used.
+        assertEquals(Instant.ofEpochMilli(1),
                      reindexing.status("one", "a").orElseThrow().ready());
 
+        // Cluster is most specific, and inherits application's common status.
         assertEquals(Instant.ofEpochMilli(1 << 20),
                      reindexing.status("one", "d").orElseThrow().ready());
 
+        // Cluster is most specific, and has its own status set.
+        assertEquals(Instant.ofEpochMilli(2 << 10),
+                     reindexing.status("two", "d").orElseThrow().ready());
+
+        // Application is most specific, as cluster and documeent are unknown.
         assertEquals(Instant.ofEpochMilli(1 << 20),
                      reindexing.status("three", "a").orElseThrow().ready());
 
@@ -45,7 +52,7 @@ public class ApplicationReindexingTest {
         assertEquals(Set.of("one", "two"),
                      reindexing.clusters().keySet());
 
-        assertEquals(new Status(Instant.EPOCH),
+        assertEquals(new Status(Instant.ofEpochMilli(1 << 20)),
                      reindexing.clusters().get("one").common());
 
         assertEquals(Map.of("a", new Status(Instant.ofEpochMilli(1))),
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 f4a553e25b7..57582040552 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
@@ -16,13 +16,14 @@ public class ReindexingMaintainerTest {
 
     @Test
     public void testReadyComputation() {
-        ApplicationReindexing reindexing = ApplicationReindexing.ready(Instant.ofEpochMilli(1 << 20))
+        ApplicationReindexing reindexing = ApplicationReindexing.ready(Instant.EPOCH)
                                                                 .withPending("one", "a", 10)
                                                                 .withReady("two", "b", Instant.ofEpochMilli(2))
                                                                 .withPending("two", "b", 20)
                                                                 .withReady("two", Instant.ofEpochMilli(2 << 10))
                                                                 .withReady("one", "a", Instant.ofEpochMilli(1))
-                                                                .withReady("two", "c", Instant.ofEpochMilli(3));
+                                                                .withReady("two", "c", Instant.ofEpochMilli(3))
+                                                                .withReady(Instant.ofEpochMilli(1 << 20));
 
         assertEquals(reindexing,
                      withReady(reindexing, () -> -1L, Instant.EPOCH));