Merge branch 'stable-3.2' into stable-3.3
* stable-3.2:
Make GlobalHealthCheck reentrant
Change-Id: Idbb21bc86bf2e0bfc021f80f1a77a4c43b512409
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
index fb298fd..cea2530 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.plugins.healthcheck;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Sets;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
@@ -54,7 +55,10 @@
@Override
public void start() {
- for (HealthCheck healthCheck : healthChecks) {
+ Set<HealthCheck> allChecks = Sets.newHashSet(healthChecks);
+ allChecks.add(globalHealthCheck);
+
+ for (HealthCheck healthCheck : allChecks) {
String name = healthCheck.name();
Counter0 failureMetric = getFailureMetric(name);
@@ -65,21 +69,6 @@
registeredMetrics.add(metricMaker.newTrigger(latencyMetric, metricCallBack));
triggers.add(metricCallBack);
}
-
- Counter0 globalFailureMetric = getFailureMetric("global");
- CallbackMetric0<Long> globalLatencyMetric = getLatencyMetric("global");
- Runnable globalMetricCallBack =
- () -> {
- HealthCheck.StatusSummary status = globalHealthCheck.getGlobalStatusSummary();
- globalLatencyMetric.set(status.elapsed);
- if (status.isFailure()) {
- globalFailureMetric.increment();
- }
- };
-
- registeredMetrics.add(globalFailureMetric);
- registeredMetrics.add(metricMaker.newTrigger(globalLatencyMetric, globalMetricCallBack));
- triggers.add(globalMetricCallBack);
}
private Runnable getCallbackMetric(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
index 9e0a607..2d5f7de 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
@@ -20,6 +20,7 @@
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
+import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result;
import java.util.Map;
import javax.servlet.http.HttpServletResponse;
@@ -36,15 +37,16 @@
@Override
public Response<Map<String, Object>> apply(ConfigResource resource)
throws AuthException, BadRequestException, ResourceConflictException, Exception {
- Map<String, Object> result = healthChecks.run();
+ HealthCheck.StatusSummary globalHealthCheckStatus = healthChecks.run();
- result.put("ts", healthChecks.getGlobalStatusSummary().ts);
- result.put("elapsed", healthChecks.getGlobalStatusSummary().elapsed);
- return Response.withStatusCode(getHTTPResultCode(result), result);
+ Map<String, Object> result = globalHealthCheckStatus.subChecks;
+ result.put("ts", globalHealthCheckStatus.ts);
+ result.put("elapsed", globalHealthCheckStatus.elapsed);
+ return Response.withStatusCode(getHTTPResultCode(globalHealthCheckStatus), result);
}
- private int getHTTPResultCode(Map<String, Object> result) {
- return healthChecks.getResultStatus(result) == HealthCheck.Result.FAILED
+ private int getHTTPResultCode(HealthCheck.StatusSummary checkStatus) {
+ return checkStatus.result == Result.FAILED
? HttpServletResponse.SC_INTERNAL_SERVER_ERROR
: HttpServletResponse.SC_OK;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
index a6eae79..d0181ab 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -17,6 +17,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig;
+import java.util.Collections;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
@@ -28,7 +29,7 @@
private final long timeout;
private final String name;
private final ListeningExecutorService executor;
- protected StatusSummary latestStatus;
+ protected volatile StatusSummary latestStatus;
protected HealthCheckConfig config;
protected AbstractHealthCheck(
@@ -47,6 +48,7 @@
@Override
public StatusSummary run() {
+ StatusSummary checkStatusSummary;
boolean enabled = config.healthCheckEnabled(name);
final long ts = System.currentTimeMillis();
ListenableFuture<StatusSummary> resultFuture =
@@ -59,18 +61,24 @@
log.warn("Check {} failed", name, e);
healthy = Result.FAILED;
}
- return new StatusSummary(healthy, ts, System.currentTimeMillis() - ts);
+ return new StatusSummary(
+ healthy, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
});
try {
- latestStatus = resultFuture.get(timeout, TimeUnit.MILLISECONDS);
+ checkStatusSummary = resultFuture.get(timeout, TimeUnit.MILLISECONDS);
} catch (TimeoutException e) {
log.warn("Check {} timed out", name, e);
- latestStatus = new StatusSummary(Result.TIMEOUT, ts, System.currentTimeMillis() - ts);
+ checkStatusSummary =
+ new StatusSummary(
+ Result.TIMEOUT, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
} catch (InterruptedException | ExecutionException e) {
log.warn("Check {} failed while waiting for its future result", name, e);
- latestStatus = new StatusSummary(Result.FAILED, ts, System.currentTimeMillis() - ts);
+ checkStatusSummary =
+ new StatusSummary(
+ Result.FAILED, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
}
- return latestStatus;
+ latestStatus = checkStatusSummary.shallowCopy();
+ return checkStatusSummary;
}
protected abstract Result doCheck() throws Exception;
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 c9066c5..c464a9e 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
@@ -23,18 +23,18 @@
import java.util.stream.StreamSupport;
@Singleton
-public class GlobalHealthCheck {
+public class GlobalHealthCheck implements HealthCheck {
private final DynamicSet<HealthCheck> healthChecks;
- private HealthCheck.StatusSummary globalStatusSummary;
+ private volatile StatusSummary latestStatus = StatusSummary.INITIAL_STATUS;
@Inject
public GlobalHealthCheck(DynamicSet<HealthCheck> healthChecks) {
this.healthChecks = healthChecks;
- this.globalStatusSummary = HealthCheck.StatusSummary.INITIAL_STATUS;
}
- public Map<String, Object> run() {
+ @Override
+ public HealthCheck.StatusSummary run() {
Iterable<HealthCheck> iterable = () -> healthChecks.iterator();
long ts = System.currentTimeMillis();
Map<String, Object> checkToResults =
@@ -42,25 +42,33 @@
.map(check -> Arrays.asList(check.name(), check.run()))
.collect(Collectors.toMap(k -> (String) k.get(0), v -> v.get(1)));
long elapsed = System.currentTimeMillis() - ts;
- globalStatusSummary =
- new HealthCheck.StatusSummary(getResultStatus(checkToResults), ts, elapsed);
- return checkToResults;
+ StatusSummary globalStatus =
+ new HealthCheck.StatusSummary(
+ hasAnyFailureOnResults(checkToResults) ? Result.FAILED : Result.PASSED,
+ ts,
+ elapsed,
+ checkToResults);
+ latestStatus = globalStatus.shallowCopy();
+ return globalStatus;
}
- public HealthCheck.StatusSummary getGlobalStatusSummary() {
- return this.globalStatusSummary;
- }
-
- public HealthCheck.Result getResultStatus(Map<String, Object> result) {
- if (result.values().stream()
+ public static boolean hasAnyFailureOnResults(Map<String, Object> results) {
+ return results.values().stream()
.filter(
res ->
res instanceof HealthCheck.StatusSummary
&& ((HealthCheck.StatusSummary) res).isFailure())
- .findFirst()
- .isPresent()) {
- return HealthCheck.Result.FAILED;
- }
- return HealthCheck.Result.PASSED;
+ .findAny()
+ .isPresent();
+ }
+
+ @Override
+ public String name() {
+ return "global";
+ }
+
+ @Override
+ public StatusSummary getLatestStatus() {
+ return latestStatus;
}
}
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 de571d1..46acdde 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
@@ -16,7 +16,9 @@
import com.google.gson.annotations.SerializedName;
import java.util.Arrays;
+import java.util.Collections;
import java.util.HashSet;
+import java.util.Map;
import java.util.Set;
public interface HealthCheck {
@@ -34,22 +36,29 @@
public class StatusSummary {
public static final StatusSummary INITIAL_STATUS =
- new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L);
+ new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L, Collections.emptyMap());
public final Result result;
public final long ts;
public final long elapsed;
+ public final transient Map<String, Object> subChecks;
+
public static final Set<Result> failingResults =
new HashSet<>(Arrays.asList(Result.FAILED, Result.TIMEOUT));
- public StatusSummary(Result result, long ts, long elapsed) {
+ public StatusSummary(Result result, long ts, long elapsed, Map<String, Object> subChecks) {
this.result = result;
this.ts = ts;
this.elapsed = elapsed;
+ this.subChecks = subChecks;
}
public Boolean isFailure() {
return failingResults.contains(this.result);
}
+
+ public StatusSummary shallowCopy() {
+ return new StatusSummary(result, ts, elapsed, Collections.emptyMap());
+ }
}
StatusSummary run();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java
deleted file mode 100644
index a1c792e..0000000
--- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java
+++ /dev/null
@@ -1,55 +0,0 @@
-// Copyright (C) 2021 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.googlesource.gerrit.plugins.healthcheck;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.inject.Inject;
-import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck;
-import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
-import java.util.HashMap;
-import org.junit.Test;
-
-public class GlobalHealthCheckTest {
-
- @Inject private DynamicSet<HealthCheck> healthChecks;
-
- GlobalHealthCheck globalHealthCheck = new GlobalHealthCheck(healthChecks);
-
- @Test
- public void shouldFailIfOneCheckFails() {
-
- HashMap<String, Object> checks =
- new HashMap<String, Object>() {
- {
- put("failingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.FAILED, 0L, 0L));
- put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L));
- }
- };
- assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.FAILED);
- }
-
- @Test
- public void shouldPassIfAllChecksPass() {
- HashMap<String, Object> checks =
- new HashMap<String, Object>() {
- {
- put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L));
- }
- };
- assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.PASSED);
- }
-}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
index fb64048..1c01950 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
@@ -33,6 +33,7 @@
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.StatusSummary;
+import java.util.Collections;
import org.junit.Before;
import org.junit.Test;
@@ -69,7 +70,7 @@
@Test
public void shouldSendCounterWhenStatusSummaryFailed() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.getFailures()).isEqualTo(1);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
@@ -78,7 +79,7 @@
@Test
public void shouldSendCounterWhenStatusSummaryTimeout() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.getFailures()).isEqualTo(1);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
@@ -87,7 +88,7 @@
@Test
public void shouldNOTSendCounterWhenStatusSummarySuccess() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.failures).isEqualTo(0L);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
