Merge pull request #359 from yrodiere/level-info

Report some failures at INFO level only

src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java

@@ -1,10 +1,30 @@
 package io.quarkus.search.app.indexing.reporting;
 
+import org.jboss.logging.Logger;
+
 public interface FailureCollector {
 
     enum Level {
-        CRITICAL,
-        WARNING;
+        /**
+         * Level for failures that prevent reindexing completely.
+         */
+        CRITICAL(Logger.Level.ERROR),
+        /**
+         * Level for failures that don't prevent reindexing but cause the index to be incomplete/incorrect,
+         * and may require maintainer's attention.
+         */
+        WARNING(Logger.Level.WARN),
+        /**
+         * Level for failures that don't prevent reindexing but cause the index to be incomplete/incorrect,
+         * though maintainer's attention is not needed (the solution is just waiting).
+         */
+        INFO(Logger.Level.INFO);
+
+        public final Logger.Level logLevel;
+
+        Level(Logger.Level logLevel) {
+            this.logLevel = logLevel;
+        }
     }
 
     enum Stage {
@@ -13,12 +33,34 @@ enum Stage {
         INDEXING;
     }
 
-    void warning(Stage stage, String details);
+    default void collect(Level level, Stage stage, String details) {
+        collect(level, stage, details, null);
+    }
+
+    void collect(Level level, Stage stage, String details, Exception exception);
+
+    default void info(Stage stage, String details) {
+        collect(Level.INFO, stage, details);
+    }
+
+    default void info(Stage stage, String details, Exception exception) {
+        collect(Level.INFO, stage, details, exception);
+    }
 
-    void warning(Stage stage, String details, Exception exception);
+    default void warning(Stage stage, String details) {
+        collect(Level.WARNING, stage, details);
+    }
 
-    void critical(Stage stage, String details);
+    default void warning(Stage stage, String details, Exception exception) {
+        collect(Level.WARNING, stage, details, exception);
+    }
 
-    void critical(Stage stage, String details, Exception exception);
+    default void critical(Stage stage, String details) {
+        collect(Level.CRITICAL, stage, details);
+    }
+
+    default void critical(Stage stage, String details, Exception exception) {
+        collect(Level.CRITICAL, stage, details, exception);
+    }
 
 }

src/main/java/io/quarkus/search/app/indexing/reporting/GithubStatusReporter.java

@@ -68,6 +68,8 @@ public void report(Status status, Map<FailureCollector.Level, List<Failure>> fai
             switch (status) {
                 case IN_PROGRESS, SUCCESS -> {
                     // When in progress or successful, never comment: we want to avoid unnecessary noise.
+                    // Note this means INFO level failures will have been logged by the FailureCollector,
+                    // but won't be reported to GitHub.
                 }
                 case WARNING -> {
                     // When warning, only comment if we didn't comment the same thing recently.

src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java

@@ -89,25 +89,9 @@ public void close() {
         }
 
         @Override
-        public void warning(Stage stage, String details) {
-            warning(stage, details, null);
-        }
-
-        @Override
-        public void warning(Stage stage, String details, Exception exception) {
-            Log.warn(details, exception);
-            failures.get(Level.WARNING).add(new Failure(Level.WARNING, stage, details, exception));
-        }
-
-        @Override
-        public void critical(Stage stage, String details) {
-            critical(stage, details, null);
-        }
-
-        @Override
-        public void critical(Stage stage, String details, Exception exception) {
-            Log.error(details, exception);
-            failures.get(Level.CRITICAL).add(new Failure(Level.CRITICAL, stage, details, exception));
+        public void collect(Level level, Stage stage, String details, Exception exception) {
+            Log.log(level.logLevel, details, exception);
+            failures.get(level).add(new Failure(level, stage, details, exception));
         }
 
         private boolean scheduleRetry() {
@@ -118,7 +102,7 @@ private boolean scheduleRetry() {
                 try {
                     scheduledRetry = retryScheduler.apply(retryConfig.delay());
                     // If we get here, a retry was scheduled.
-                    warning(Stage.INDEXING, "Indexing will be tried again later.");
+                    info(Stage.INDEXING, "Indexing will be tried again later.");
                     return true;
                 } catch (RuntimeException e) {
                     // If we get here, we'll abort.
@@ -133,6 +117,8 @@ private boolean scheduleRetry() {
         }
 
         private static Status indexingResultStatus(Map<Level, List<Failure>> failures) {
+            // INFO level is ignored for the overall status.
+            // We will only report INFO failures if there are other CRITICAL/WARNING failures.
             if (failures.get(Level.CRITICAL).isEmpty()) {
                 if (failures.get(Level.WARNING).isEmpty()) {
                     return Status.SUCCESS;

src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java

@@ -7,6 +7,7 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.time.Duration;
 import java.time.Instant;
@@ -137,7 +138,7 @@ private static void validateRepositories(
                 //  more than 7 days; so we give it 2 weeks period before we start considering that there's a sync problem:
                 Duration duration = Duration.between(lastSync, Instant.now());
                 if (duration.compareTo(Duration.ofDays(14)) > 0) {
-                    failureCollector.warning(
+                    failureCollector.info(
                             FailureCollector.Stage.TRANSLATION,
                             "Localized repository '" + localized.getKey() + "' is out of sync for " + duration);
                 }
@@ -153,7 +154,7 @@ private static void validateRepositories(
     @Override
     public void close() throws IOException {
         for (QuarkusIOCloneDirectory directory : allSites.values()) {
-            directory.unprocessed().ifPresent(m -> failureCollector.warning(FailureCollector.Stage.PARSING, m));
+            directory.unprocessed().ifPresent(m -> failureCollector.info(FailureCollector.Stage.PARSING, m));
         }
         try (var closer = new Closer<IOException>()) {
             closer.push(CloseableDirectory::close, prefetchedGuides);
@@ -447,12 +448,17 @@ private Catalog translations(Repository repository, RevTree sources, String path
 
         try (InputStream file = GitUtils.file(repository, sources, path)) {
             return new PoParser().parseCatalog(file, false);
-        } catch (IOException | IllegalStateException e) {
-            // it may be that not all localized sites are up-to-date, in that case we just assume that the translation is not there
-            // and the non-translated english text will be used.
+        } catch (NoSuchFileException e) {
+            // translation may be missing, but that's just the way it is: it'll get translated when someone has time
+            failureCollector.info(FailureCollector.Stage.TRANSLATION,
+                    "Unable to open translation file " + path + " : " + e.getMessage(), e);
+            // in the meantime we'll use non-translated English text
+            return new Catalog();
+        } catch (IOException e) {
+            // opening/parsing may fail, in which case we definitely need to have a look
             failureCollector.warning(FailureCollector.Stage.TRANSLATION,
-                    "Unable to parse a translation file " + path + " : " + e.getMessage(), e);
-
+                    "Unable to parse translation file " + path + " : " + e.getMessage(), e);
+            // in the meantime we'll use non-translated English text
             return new Catalog();
         }
     }
@@ -555,11 +561,11 @@ private Guide createCoreGuide(GitCloneDirectory cloneDirectory, String quarkusVe
             // if  a file is not present we do not want to add such guide. Since if the html is not there
             // it means that users won't be able to open it on the site, and returning it in the search results make it pointless.
 
-            // Since this file was listed in the yaml of a rev from which the site was built the html should've been present,
-            // but is missing for some reason that needs investigation:
-            failureCollector.warning(
+            // Since this file was listed in the yaml of a rev from which the site was built, the html should've been present.
+            // So if it's missing, that's important context when investigating other errors.
+            failureCollector.info(
                     FailureCollector.Stage.TRANSLATION,
-                    "Guide " + guide + " is ignored since we were not able to find an HTML content file for it.");
+                    guide + " is ignored since we were not able to find an HTML content file for it.");
             return null;
         }
 

src/main/java/io/quarkus/search/app/util/GitUtils.java

@@ -3,6 +3,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.NoSuchFileException;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -59,7 +60,7 @@ public static InputStream file(Repository repo, RevTree tree, String path) throw
         treeWalk.setRecursive(true);
         treeWalk.setFilter(PathFilter.create(path));
         if (!treeWalk.next()) {
-            throw new IllegalStateException("Missing file '%s' in '%s'".formatted(path, tree));
+            throw new NoSuchFileException("Missing file '%s' in '%s'".formatted(path, tree));
         }
         ObjectId objectId = treeWalk.getObjectId(0);
         ObjectLoader loader = repo.open(objectId);

src/test/java/io/quarkus/search/app/indexing/state/IndexingStateTest.java

@@ -45,6 +45,9 @@ void success() {
             assertThat(state.isInProgress()).isTrue();
             verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));
 
+            // info should not affect the status
+            attempt.info(Stage.INDEXING, "Some info");
+
             // No warning/critical: success
         }
         verify(statusReporterMock).report(eq(Status.SUCCESS), anyMap());
@@ -80,6 +83,9 @@ void warning() {
             verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));
 
             attempt.warning(Stage.INDEXING, "Some warning");
+
+            // info should not affect the status
+            attempt.info(Stage.INDEXING, "Some info");
         }
         verify(statusReporterMock).report(eq(Status.WARNING), anyMap());
         assertThat(state.isInProgress()).isFalse();
@@ -96,6 +102,9 @@ void failure_max1Attempt() {
             verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));
 
             attempt.critical(Stage.INDEXING, "Something critical");
+
+            // info should not affect the status
+            attempt.info(Stage.INDEXING, "Some info");
         }
         verify(statusReporterMock).report(eq(Status.CRITICAL), anyMap());
         assertThat(state.isInProgress()).isFalse();