Merge branch 'stable-3.10' * stable-3.10: Short-circuit on failed healthchecks Simplify construction of map of check results Simplify check for any failures Run checks in parallel Change-Id: I30c16e082fca33af3ae82d610d8e5191fdaf584d
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java index fa5f14a..4e4cef7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java
@@ -22,9 +22,11 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; -import java.util.Arrays; +import java.util.Collections; import java.util.Map; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.StreamSupport; @@ -33,6 +35,31 @@ private final DynamicSet<HealthCheck> healthChecks; + public static class MemoizedStatusSummary implements Supplier<StatusSummary> { + private final AtomicReference<StatusSummary> result = new AtomicReference<>(); + private final HealthCheck check; + + MemoizedStatusSummary(HealthCheck check) { + this.check = check; + } + + @Override + public StatusSummary get() { + if (result.get() == null) { + result.set(check.run()); + } + return result.get(); + } + + public StatusSummary getIfCompleted() { + StatusSummary completedResult = result.get(); + return completedResult == null + ? new StatusSummary( + Result.NOT_RUN, System.currentTimeMillis(), 0L, Collections.emptyMap()) + : completedResult; + } + } + @Inject public GlobalHealthCheck( DynamicSet<HealthCheck> healthChecks, @@ -47,17 +74,16 @@ public HealthCheck.StatusSummary run() { Iterable<HealthCheck> iterable = () -> healthChecks.iterator(); long ts = System.currentTimeMillis(); - Map<String, Object> checkToResults = + Map<String, MemoizedStatusSummary> checkToResults = StreamSupport.stream(iterable.spliterator(), false) - .map(check -> Arrays.asList(check.name(), check.run())) - .collect(Collectors.toMap(k -> (String) k.get(0), v -> v.get(1))); + .collect(Collectors.toMap(HealthCheck::name, MemoizedStatusSummary::new)); long elapsed = System.currentTimeMillis() - ts; + Result checkResult = hasAnyFailureOnResults(checkToResults) ? Result.FAILED : Result.PASSED; + Map<String, Object> reportedResults = + checkToResults.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, v -> v.getValue().getIfCompleted())); StatusSummary globalStatus = - new HealthCheck.StatusSummary( - hasAnyFailureOnResults(checkToResults) ? Result.FAILED : Result.PASSED, - ts, - elapsed, - checkToResults); + new HealthCheck.StatusSummary(checkResult, ts, elapsed, reportedResults); if (globalStatus.isFailure()) { failureCounterMetric.increment(); } @@ -70,13 +96,7 @@ return run().result; } - public static boolean hasAnyFailureOnResults(Map<String, Object> results) { - return results.values().stream() - .filter( - res -> - res instanceof HealthCheck.StatusSummary - && ((HealthCheck.StatusSummary) res).isFailure()) - .findAny() - .isPresent(); + public static boolean hasAnyFailureOnResults(Map<String, MemoizedStatusSummary> results) { + return results.values().stream().parallel().anyMatch(res -> res.get().isFailure()); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java index 7c4f8a1..db07320 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
@@ -30,6 +30,8 @@ FAILED, @SerializedName("timeout") TIMEOUT, + @SerializedName("not_run") + NOT_RUN, @SerializedName("disabled") DISABLED; }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java index 56f96fe..1b1cd70 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java
@@ -16,13 +16,20 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.extensions.annotations.PluginName; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.metrics.MetricMaker; import com.google.gson.Gson; import com.google.gson.JsonObject; import com.google.inject.AbstractModule; +import com.google.inject.Inject; import com.google.inject.Key; +import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.healthcheck.check.AbstractHealthCheck; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames; import java.io.IOException; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -34,6 +41,33 @@ protected void configure() { install(new HealthCheckExtensionApiModule()); install(new Module()); + DynamicSet.bind(binder(), HealthCheck.class).to(FakeHealthCheck.class); + } + } + + @Singleton + public static class FakeHealthCheck extends AbstractHealthCheck { + private long sleep = 0L; + private Result result = Result.PASSED; + + @Inject + public FakeHealthCheck( + ListeningExecutorService executor, HealthCheckConfig config, MetricMaker metricMaker) { + super(executor, config, "fake-check", metricMaker); + } + + public void setSleep(long sleep) { + this.sleep = sleep; + } + + public void setResult(Result result) { + this.result = result; + } + + @Override + protected Result doCheck() throws Exception { + Thread.sleep(sleep); + return result; } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java index 86ce328..621ba17 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
@@ -26,9 +26,15 @@ import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gson.JsonElement; import com.google.gson.JsonObject; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; import java.io.File; import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletResponse; import org.eclipse.jgit.errors.ConfigInvalidException; import org.junit.Before; @@ -114,6 +120,25 @@ } @Test + public void shouldNotRunChecksWhenOneFails() throws Exception { + FakeHealthCheck fakeHealthCheck = plugin.getSysInjector().getInstance(FakeHealthCheck.class); + fakeHealthCheck.setResult(HealthCheck.Result.FAILED); + + RestResponse resp = getHealthCheckStatus(); + resp.assertStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + JsonObject respJson = getResponseJson(resp); + Set<Map.Entry<String, JsonElement>> checkResults = respJson.entrySet(); + List<String> results = + checkResults.stream() + .map(Map.Entry::getValue) + .filter(JsonElement::isJsonObject) + .map(j -> j.getAsJsonObject().get("result")) + .map(JsonElement::getAsString) + .collect(Collectors.toList()); + assertThat(results).contains("not_run"); + } + + @Test public void shouldReturnAuthCheckAsDisabled() throws Exception { disableCheck(AUTH); RestResponse resp = getHealthCheckStatus();