From 7238a5fae1184cb337c3b65598b2f34865e61742 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= <yoann@hibernate.org>
Date: Fri, 20 Oct 2023 23:28:25 +0200
Subject: [PATCH] Make sure search functionality is restored immediately upon
 rollover recovery

---
 .../quarkus/search/app/indexing/Rollover.java | 52 +++++++++++--------
 .../search/app/indexing/RolloverTest.java     | 20 ++++---
 2 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/src/main/java/io/quarkus/search/app/indexing/Rollover.java b/src/main/java/io/quarkus/search/app/indexing/Rollover.java
index e3dd80ba..4e64ad0c 100644
--- a/src/main/java/io/quarkus/search/app/indexing/Rollover.java
+++ b/src/main/java/io/quarkus/search/app/indexing/Rollover.java
@@ -218,11 +218,7 @@ public void addActualIndex(String name, boolean isWrite) {
             }
         }
 
-        public boolean hasMultipleIndexes() {
-            return allAliasedIndexes.size() > 1;
-        }
-
-        Set<String> allAliasedIndexes() {
+        public Set<String> allAliasedIndexes() {
             return Collections.unmodifiableSet(allAliasedIndexes);
         }
 
@@ -230,23 +226,9 @@ Set<String> writeAliasedIndexes() {
             return Collections.unmodifiableSet(writeAliasedIndexes);
         }
 
-        Set<String> readAliasedIndexes() {
+        public Set<String> readAliasedIndexes() {
             return Collections.unmodifiableSet(readAliasedIndexes);
         }
-
-        public Set<String> extraIndexes() {
-            Set<String> extra = new HashSet<>(allAliasedIndexes);
-            // We keep only the oldest write index,
-            // which hopefully should allow a rollover to start.
-            if (!writeAliasedIndexes.isEmpty()) {
-                extra.remove(writeAliasedIndexes.iterator().next());
-            }
-            // Failing that, we keep only one index, and hope for the best.
-            else if (!allAliasedIndexes.isEmpty()) {
-                extra.remove(allAliasedIndexes.iterator().next());
-            }
-            return extra;
-        }
     }
 
     private static void commitAll(RestClient client, Gson gson, List<IndexRolloverResult> rollovers) {
@@ -286,7 +268,7 @@ private static void rollbackAll(RestClient client, Gson gson, List<IndexRollover
     private static boolean recoverInconsistentAliases(RestClient client, Gson gson,
             Collection<GetAliasedResult> aliased) {
         List<GetAliasedResult> inconsistentList = aliased.stream()
-                .filter(GetAliasedResult::hasMultipleIndexes)
+                .filter(a -> a.allAliasedIndexes.size() > 1)
                 .toList();
         if (inconsistentList.isEmpty()) {
             return false;
@@ -294,8 +276,32 @@ private static boolean recoverInconsistentAliases(RestClient client, Gson gson,
         List<String> inconsistentNames = inconsistentList.stream().map(r -> r.index.hibernateSearchName()).toList();
         Log.infof("Recovering index aliases for %s", inconsistentNames);
         try {
-            changeAliasesAtomically(client, gson, inconsistentList, inconsistent -> inconsistent.extraIndexes().stream()
-                    .map(indexName -> aliasAction("remove_index", Map.of("index", indexName))));
+            changeAliasesAtomically(client, gson, inconsistentList, inconsistent -> {
+                Set<String> extraIndexes = new HashSet<>(inconsistent.allAliasedIndexes);
+                String indexToKeep;
+                // We keep only the oldest read index,
+                // which hopefully should restore a complete index with the ability to search.
+                if (!inconsistent.readAliasedIndexes.isEmpty()) {
+                    indexToKeep = inconsistent.readAliasedIndexes.iterator().next();
+                }
+                // Failing that, we keep only one write, and hope for the best.
+                else {
+                    indexToKeep = inconsistent.allAliasedIndexes.iterator().next();
+                }
+                extraIndexes.remove(indexToKeep);
+                JsonObject restoreIndexToKeepAsWriteIndex = aliasAction("add", Map.of(
+                        "index", indexToKeep,
+                        "alias", inconsistent.index.writeName(),
+                        "is_write_index", "true"));
+                JsonObject restoreIndexToKeepAsReadIndex = aliasAction("add", Map.of(
+                        "index", indexToKeep,
+                        "alias", inconsistent.index.readName(),
+                        "is_write_index", "false"));
+                return Stream.concat(
+                        Stream.of(restoreIndexToKeepAsWriteIndex, restoreIndexToKeepAsReadIndex),
+                        extraIndexes.stream()
+                                .map(indexName -> aliasAction("remove_index", Map.of("index", indexName))));
+            });
         } catch (RuntimeException | IOException e) {
             throw new IllegalStateException("Failed to recover index aliases for " + inconsistentNames + ": " + e.getMessage(),
                     e);
diff --git a/src/test/java/io/quarkus/search/app/indexing/RolloverTest.java b/src/test/java/io/quarkus/search/app/indexing/RolloverTest.java
index 87b93d69..9cd3063f 100644
--- a/src/test/java/io/quarkus/search/app/indexing/RolloverTest.java
+++ b/src/test/java/io/quarkus/search/app/indexing/RolloverTest.java
@@ -154,23 +154,27 @@ void recover() {
         // Simulate a JVM crash: the rollover does not get closed
         // We restart... and try to recover
         assertThat(Rollover.recoverInconsistentAliases(searchMapping)).isTrue();
-        // Immediately after recovery, search may not work, but write will
-        assertWriteTargetsIndex(2);
+        // After recovery and throughout indexing, search must continue to work,
+        // and target the index most likely to contain (complete) data.
+        assertSearchWorksAndTargetsIndex(1);
+        assertWriteTargetsIndex(1);
 
         // Then we will reindex, triggering another rollover...
         searchMapping.scope(Guide.class).schemaManager().createIfMissing();
-        assertWriteTargetsIndex(2);
+        assertSearchWorksAndTargetsIndex(1);
+        assertWriteTargetsIndex(1);
 
         try (Rollover rollover = Rollover.start(searchMapping)) {
-            assertWriteTargetsIndex(3);
+            assertSearchWorksAndTargetsIndex(1);
+            assertWriteTargetsIndex(2);
 
             rollover.commit();
-            assertSearchWorksAndTargetsIndex(3);
-            assertWriteTargetsIndex(3);
+            assertSearchWorksAndTargetsIndex(2);
+            assertWriteTargetsIndex(2);
         }
         // And all is good in the end!
-        assertSearchWorksAndTargetsIndex(3);
-        assertWriteTargetsIndex(3);
+        assertSearchWorksAndTargetsIndex(2);
+        assertWriteTargetsIndex(2);
     }
 
 }