From 3d698e410e30ab63d0a5e7c0bbe385793bfb1343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Fri, 8 Nov 2024 09:11:49 +0100 Subject: [PATCH 1/3] Simplify FailureCollector implementation --- .../indexing/reporting/FailureCollector.java | 41 ++++++++++++++++--- .../app/indexing/state/IndexingState.java | 26 ++---------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java b/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java index 2349535..778fa38 100644 --- a/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java +++ b/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java @@ -1,10 +1,25 @@ 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); + + public final Logger.Level logLevel; + + Level(Logger.Level logLevel) { + this.logLevel = logLevel; + } } enum Stage { @@ -13,12 +28,26 @@ 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 warning(Stage stage, String details) { + collect(Level.WARNING, stage, details); + } - void warning(Stage stage, String details, Exception exception); + default void warning(Stage stage, String details, Exception exception) { + collect(Level.WARNING, stage, details, exception); + } - void critical(Stage stage, String details); + default void critical(Stage stage, String details) { + collect(Level.CRITICAL, stage, details); + } - void critical(Stage stage, String details, Exception exception); + default void critical(Stage stage, String details, Exception exception) { + collect(Level.CRITICAL, stage, details, exception); + } } diff --git a/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java b/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java index 73914e6..a9bf76f 100644 --- a/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java +++ b/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java @@ -11,13 +11,11 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import io.quarkus.logging.Log; import io.quarkus.search.app.indexing.reporting.Failure; import io.quarkus.search.app.indexing.reporting.FailureCollector; import io.quarkus.search.app.indexing.reporting.Status; import io.quarkus.search.app.indexing.reporting.StatusReporter; - -import io.quarkus.logging.Log; - import io.smallrye.mutiny.subscription.Cancellable; public class IndexingState { @@ -89,25 +87,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() { From 1c29b94b0857a8b30d61d0cc60e3c891e88090a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Fri, 8 Nov 2024 09:28:30 +0100 Subject: [PATCH 2/3] Report some failures at INFO level only And don't report them on GitHub if there is nothing more serious, to create less noise with GitHub notifications. --- .../indexing/reporting/FailureCollector.java | 15 ++++++++++- .../reporting/GithubStatusReporter.java | 2 ++ .../app/indexing/state/IndexingState.java | 8 ++++-- .../search/app/quarkusio/QuarkusIO.java | 26 ++++++++++++------- .../io/quarkus/search/app/util/GitUtils.java | 3 ++- .../app/indexing/state/IndexingStateTest.java | 9 +++++++ 6 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java b/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java index 778fa38..478a9dc 100644 --- a/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java +++ b/src/main/java/io/quarkus/search/app/indexing/reporting/FailureCollector.java @@ -13,7 +13,12 @@ enum Level { * 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); + 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; @@ -34,6 +39,14 @@ default void collect(Level level, Stage stage, String details) { 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); + } + default void warning(Stage stage, String details) { collect(Level.WARNING, stage, details); } diff --git a/src/main/java/io/quarkus/search/app/indexing/reporting/GithubStatusReporter.java b/src/main/java/io/quarkus/search/app/indexing/reporting/GithubStatusReporter.java index 5b44ab6..ab6f5cf 100644 --- a/src/main/java/io/quarkus/search/app/indexing/reporting/GithubStatusReporter.java +++ b/src/main/java/io/quarkus/search/app/indexing/reporting/GithubStatusReporter.java @@ -68,6 +68,8 @@ public void report(Status status, Map> 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. diff --git a/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java b/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java index a9bf76f..d8c4955 100644 --- a/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java +++ b/src/main/java/io/quarkus/search/app/indexing/state/IndexingState.java @@ -11,11 +11,13 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; -import io.quarkus.logging.Log; import io.quarkus.search.app.indexing.reporting.Failure; import io.quarkus.search.app.indexing.reporting.FailureCollector; import io.quarkus.search.app.indexing.reporting.Status; import io.quarkus.search.app.indexing.reporting.StatusReporter; + +import io.quarkus.logging.Log; + import io.smallrye.mutiny.subscription.Cancellable; public class IndexingState { @@ -100,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. @@ -115,6 +117,8 @@ private boolean scheduleRetry() { } private static Status indexingResultStatus(Map> 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; diff --git a/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java b/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java index 7577982..3f3185a 100644 --- a/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java +++ b/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()) { 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,9 +561,9 @@ 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."); return null; diff --git a/src/main/java/io/quarkus/search/app/util/GitUtils.java b/src/main/java/io/quarkus/search/app/util/GitUtils.java index 3cb7c46..66ecd49 100644 --- a/src/main/java/io/quarkus/search/app/util/GitUtils.java +++ b/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); diff --git a/src/test/java/io/quarkus/search/app/indexing/state/IndexingStateTest.java b/src/test/java/io/quarkus/search/app/indexing/state/IndexingStateTest.java index a408b1a..30d2a19 100644 --- a/src/test/java/io/quarkus/search/app/indexing/state/IndexingStateTest.java +++ b/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(); From 9299751eadf72632cac47ae07af596f5bf1e821f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Fri, 8 Nov 2024 09:37:23 +0100 Subject: [PATCH 3/3] Fix weird formatting in missing guide warnings We don't want this: > Guide Guide{url=https://cn.quarkus.io/guides/extension-faq} is ignored since we were not able to find an HTML content file for it. "Guide" is repeated, that's weird. We want this: > Guide{url=https://cn.quarkus.io/guides/extension-faq} is ignored since we were not able to find an HTML content file for it. --- src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java b/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java index 3f3185a..2dcbdc0 100644 --- a/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java +++ b/src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java @@ -565,7 +565,7 @@ private Guide createCoreGuide(GitCloneDirectory cloneDirectory, String quarkusVe // 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; }