Populate CombinedCheckState in query path from a cache
We want to be able to serve combined check state in query results
cheaply in the common case. Recomputing the state from scratch is
expensive, as it requires:
* opening the change repo
* reading the check ref and note contents for the current patch set
* reading checker configs for the repo
* running checker queries to determine if the check state should be
backfilled.
Introduce a mutable, optionally persisted cache keyed only by project
and change/patch set ID that stores the combined check state. This is
the simplest key that allows us to fill in many results quickly. Use the
"--combined" option in the query path as well.
To mitigate potential staleness in search results, put results into the
cache anytime the check state is updated or the combined check state is
recomputed from primary storage. To avoid too many writes to persistent
storage, we do a racy read-then-write and only update the cached value
if it differs. The assumption is that most updates to individual checks
will not result in the combined state changing. The raciness is
mitigated by the fact that we do this dance on every change screen load,
so it should converge to the right value regularly.
Include a metric recording latency of the reload step, keeping track of
whether the recomputation resulted in a write to the underlying cache.
The tests involving the metric highlight the fact that there are already
multiple places in the request path where we recompute the combined
check state. Exploring ways to reduce that duplicate work is beyond the
scope of this change. In the context of this change, at most one of the
duplicate recomputations will result in updating the cache.
Change-Id: I8ce16c979a0fc5697b0dd604e150481ef661fa16
diff --git a/BUILD b/BUILD
index bfbc89d..c28bc66 100644
--- a/BUILD
+++ b/BUILD
@@ -21,4 +21,5 @@
resource_jars = ["//plugins/checks/gr-checks:gr-checks-static"],
resource_strip_prefix = "plugins/checks/resources",
resources = glob(["resources/**/*"]),
+ deps = ["//plugins/checks/proto:cache_java_proto"],
)
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index 4c80cd4..40a9c4c 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -65,6 +65,10 @@
/**
* Returns the combined check state of a given patch set.
*
+ * <p>Most callers should prefer {@link
+ * com.google.gerrit.plugins.checks.CombinedCheckStateCache#reload} to automatically fix up the
+ * cache in case primary storage differs from the cached value.
+ *
* @param projectName the name of the project.
* @param patchSetId the ID of the patch set
* @return the {@link CombinedCheckState} of the current patch set.
diff --git a/java/com/google/gerrit/plugins/checks/CombinedCheckStateCache.java b/java/com/google/gerrit/plugins/checks/CombinedCheckStateCache.java
new file mode 100644
index 0000000..c348550
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/CombinedCheckStateCache.java
@@ -0,0 +1,243 @@
+// Copyright (C) 2019 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.google.gerrit.plugins.checks;
+
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Stopwatch;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.CacheStats;
+import com.google.common.cache.LoadingCache;
+import com.google.common.flogger.FluentLogger;
+import com.google.common.util.concurrent.AtomicLongMap;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Description.Units;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.metrics.Timer1;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.plugins.checks.cache.proto.Cache.CombinedCheckStateCacheKeyProto;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.serialize.EnumCacheSerializer;
+import com.google.gerrit.server.cache.serialize.ProtobufSerializer;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+import java.io.IOException;
+import java.time.Duration;
+import java.util.concurrent.ExecutionException;
+
+/**
+ * Cache of {@link CombinedCheckState} per change.
+ *
+ * <p>In the absence of plugin-defined index fields, this cache is used to performantly populate the
+ * {@code combinedState} field in {@code ChangeCheckInfo} in the query path.
+ */
+@Singleton
+public class CombinedCheckStateCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final String NAME = "combined_check_state";
+
+ public static Module module() {
+ return new CacheModule() {
+ @Override
+ public void configure() {
+ persist(NAME, CombinedCheckStateCacheKeyProto.class, CombinedCheckState.class)
+ .version(1)
+ .maximumWeight(10000)
+ .diskLimit(-1)
+ .keySerializer(new ProtobufSerializer<>(CombinedCheckStateCacheKeyProto.parser()))
+ .valueSerializer(new EnumCacheSerializer<>(CombinedCheckState.class))
+ .loader(Loader.class);
+ }
+ };
+ }
+
+ @Singleton
+ static class Metrics {
+ // Pair of metric and manual counters, to work around the fact that metric classes have no
+ // getters.
+ private final Timer1<Boolean> reloadLatency;
+ private final AtomicLongMap<Boolean> reloadCount;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ reloadLatency =
+ metricMaker.newTimer(
+ "checks/reload_combined_check_state",
+ new Description("Latency for reloading combined check state")
+ .setCumulative()
+ .setUnit(Units.MILLISECONDS),
+ Field.ofBoolean(
+ "updated", "whether reloading resulted in updating the cached value"));
+ reloadCount = AtomicLongMap.create();
+ }
+
+ void recordReload(boolean updated, Duration elapsed) {
+ reloadLatency.record(updated, elapsed.toNanos(), NANOSECONDS);
+ reloadCount.incrementAndGet(updated);
+ }
+
+ long getReloadCount(boolean updated) {
+ return reloadCount.get(updated);
+ }
+ }
+
+ private final LoadingCache<CombinedCheckStateCacheKeyProto, CombinedCheckState> cache;
+ private final Loader loader;
+ private final Metrics metrics;
+
+ @Inject
+ CombinedCheckStateCache(
+ @Named(NAME) LoadingCache<CombinedCheckStateCacheKeyProto, CombinedCheckState> cache,
+ Loader loader,
+ Metrics metrics) {
+ this.cache = cache;
+ this.loader = loader;
+ this.metrics = metrics;
+ }
+
+ /**
+ * Get the state from the cache, computing it from checks ref if necessary.
+ *
+ * @param project project containing the change.
+ * @param psId patch set to which the state corresponds.
+ * @return combined check state.
+ */
+ public CombinedCheckState get(Project.NameKey project, PatchSet.Id psId) {
+ try {
+ return cache.get(key(project, psId));
+ } catch (ExecutionException e) {
+ throw new StorageException(e);
+ }
+ }
+
+ /**
+ * Load the state from primary storage, and update the state in the cache only if it changed.
+ *
+ * <p>This method does a cache lookup followed by a write, which is inherently racy. It is
+ * intended to be used whenever we need to recompute the combined check state, for example when
+ * sending a {@code ChangeCheckInfo} to the client. As a result, inconsistencies between the cache
+ * and the actual state should tend to get fixed up immediately after a user views the change.
+ *
+ * @param project project containing the change.
+ * @param psId patch set to which the state corresponds.
+ * @return combined check state.
+ */
+ public CombinedCheckState reload(Project.NameKey project, PatchSet.Id psId) {
+ // Possible future optimization: short-circuit before calling this method, if an individual
+ // check transitioned between two CheckStates which would result in the same CombinedCheckState.
+ Stopwatch sw = Stopwatch.createStarted();
+ // Arbitrarily assume that the cache was updated unless we can conclusively prove it wasn't.
+ boolean updated = true;
+ try {
+ CombinedCheckStateCacheKeyProto key = key(project, psId);
+ CombinedCheckState newState = loader.load(key);
+ CombinedCheckState oldState = cache.getIfPresent(key);
+ if (newState != oldState) {
+ cache.put(key, newState);
+ } else {
+ updated = false;
+ }
+ return newState;
+ } finally {
+ metrics.recordReload(updated, sw.elapsed());
+ }
+ }
+
+ /**
+ * Update the state in the cache only if it changed.
+ *
+ * <p>This method returns no value and should be called when the caller may have recently updated
+ * the value in primary storage, but does not need the actual value. There is no guarantee that an
+ * update initiated by this method completes synchronously.
+ *
+ * <p>This method does a cache lookup followed by a write, which is inherently racy.
+ * Inconsistencies between the cache and the actual state should tend to get fixed up immediately
+ * after a user views the change, since the read path calls {@link #reload(Project.NameKey,
+ * PatchSet.Id)}.
+ *
+ * @param project project containing the change.
+ * @param psId patch set to which the state corresponds.
+ */
+ public void updateIfNecessary(Project.NameKey project, PatchSet.Id psId) {
+ // Possible future optimization: make this whole method async, in the FanOutExecutor.
+ try {
+ reload(project, psId);
+ } catch (RuntimeException e) {
+ logger.atWarning().withCause(e).log(
+ "failed to reload CombinedCheckState for %s in %s", psId, project);
+ }
+ }
+
+ /**
+ * Directly put a state into the cache.
+ *
+ * @param project project containing the change.
+ * @param psId patch set to which the state corresponds.
+ * @param state combined check state.
+ */
+ @VisibleForTesting
+ public void putForTest(Project.NameKey project, PatchSet.Id psId, CombinedCheckState state) {
+ cache.put(key(project, psId), state);
+ }
+
+ @VisibleForTesting
+ public long getReloadCount(boolean updated) {
+ return metrics.getReloadCount(updated);
+ }
+
+ @VisibleForTesting
+ public CacheStats getStats() {
+ return cache.stats();
+ }
+
+ private static CombinedCheckStateCacheKeyProto key(Project.NameKey project, PatchSet.Id psId) {
+ return CombinedCheckStateCacheKeyProto.newBuilder()
+ .setProject(project.get())
+ .setChangeId(psId.changeId().get())
+ .setPatchSetId(psId.get())
+ .build();
+ }
+
+ @Singleton
+ private static class Loader
+ extends CacheLoader<CombinedCheckStateCacheKeyProto, CombinedCheckState> {
+ private final Checks checks;
+
+ @Inject
+ Loader(Checks checks) {
+ this.checks = checks;
+ }
+
+ @Override
+ public CombinedCheckState load(CombinedCheckStateCacheKeyProto key) {
+ try {
+ return checks.getCombinedCheckState(
+ Project.nameKey(key.getProject()),
+ PatchSet.id(Change.id(key.getChangeId()), key.getPatchSetId()));
+ } catch (IOException e) {
+ throw new StorageException(e);
+ }
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/Module.java b/java/com/google/gerrit/plugins/checks/Module.java
index 102c057..8440501 100644
--- a/java/com/google/gerrit/plugins/checks/Module.java
+++ b/java/com/google/gerrit/plugins/checks/Module.java
@@ -23,6 +23,7 @@
import com.google.gerrit.plugins.checks.api.ApiModule;
import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory;
import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory.GetChangeOptions;
+import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory.QueryChangesOptions;
import com.google.gerrit.plugins.checks.db.NoteDbCheckersModule;
import com.google.gerrit.plugins.checks.rules.ChecksSubmitRule;
import com.google.gerrit.server.DynamicOptions;
@@ -31,12 +32,15 @@
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.gerrit.server.restapi.change.GetChange;
+import com.google.gerrit.server.restapi.change.QueryChanges;
+import com.google.gerrit.sshd.commands.Query;
public class Module extends FactoryModule {
@Override
protected void configure() {
factory(CheckJson.AssistedFactory.class);
install(new NoteDbCheckersModule());
+ install(CombinedCheckStateCache.module());
bind(CapabilityDefinition.class)
.annotatedWith(Exports.named(AdministrateCheckersCapability.NAME))
@@ -56,6 +60,12 @@
bind(DynamicOptions.DynamicBean.class)
.annotatedWith(Exports.named(GetChange.class))
.to(GetChangeOptions.class);
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(Exports.named(QueryChanges.class))
+ .to(QueryChangesOptions.class);
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(Exports.named(Query.class))
+ .to(QueryChangesOptions.class);
install(new ApiModule());
install(new ChecksSubmitRule.Module());
diff --git a/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java b/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java
index d2f76b0..9c48851 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChangeCheckAttributeFactory.java
@@ -14,15 +14,13 @@
package com.google.gerrit.plugins.checks.api;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.server.DynamicOptions.BeanProvider;
import com.google.gerrit.server.DynamicOptions.DynamicBean;
import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.io.IOException;
import org.kohsuke.args4j.Option;
/**
@@ -31,16 +29,24 @@
*/
@Singleton
public class ChangeCheckAttributeFactory implements ChangeAttributeFactory {
+ private static final String COMBINED_OPTION_NAME = "--combined";
+ private static final String COMBINED_OPTION_USAGE = "include combined check state";
+
public static class GetChangeOptions implements DynamicBean {
- @Option(name = "--combined", usage = "include combined check state")
+ @Option(name = COMBINED_OPTION_NAME, usage = COMBINED_OPTION_USAGE)
boolean combined;
}
- private final Checks checks;
+ public static class QueryChangesOptions implements DynamicBean {
+ @Option(name = COMBINED_OPTION_NAME, usage = COMBINED_OPTION_USAGE)
+ boolean combined;
+ }
+
+ private final CombinedCheckStateCache combinedCheckStateCache;
@Inject
- ChangeCheckAttributeFactory(Checks checks) {
- this.checks = checks;
+ ChangeCheckAttributeFactory(CombinedCheckStateCache combinedCheckStateCache) {
+ this.combinedCheckStateCache = combinedCheckStateCache;
}
@Override
@@ -49,25 +55,28 @@
if (opts == null) {
return null;
}
- try {
- if (opts instanceof GetChangeOptions) {
- return forGetChange(cd, (GetChangeOptions) opts);
- }
- // TODO(dborowitz): Compute from cache in query path.
- } catch (StorageException | IOException e) {
- throw new RuntimeException(e);
+ if (opts instanceof GetChangeOptions) {
+ return forGetChange(cd, (GetChangeOptions) opts);
+ } else if (opts instanceof QueryChangesOptions) {
+ return forQueryChanges(cd, (QueryChangesOptions) opts);
}
throw new IllegalStateException("unexpected options type: " + opts);
}
- private ChangeCheckInfo forGetChange(ChangeData cd, GetChangeOptions opts)
- throws StorageException, IOException {
+ private ChangeCheckInfo forGetChange(ChangeData cd, GetChangeOptions opts) {
if (opts == null || !opts.combined) {
return null;
}
- ChangeCheckInfo info = new ChangeCheckInfo();
- info.combinedState =
- checks.getCombinedCheckState(cd.project(), cd.change().currentPatchSetId());
- return info;
+ // Reload value in cache to fix up inconsistencies between cache and actual state.
+ return new ChangeCheckInfo(
+ combinedCheckStateCache.reload(cd.project(), cd.change().currentPatchSetId()));
+ }
+
+ private ChangeCheckInfo forQueryChanges(ChangeData cd, QueryChangesOptions opts) {
+ if (!opts.combined) {
+ return null;
+ }
+ return new ChangeCheckInfo(
+ combinedCheckStateCache.get(cd.project(), cd.change().currentPatchSetId()));
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java b/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java
index 257b8fc..2089bb5 100644
--- a/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/ChangeCheckInfo.java
@@ -27,11 +27,15 @@
public ChangeCheckInfo() {}
- public ChangeCheckInfo(String pluginName, CombinedCheckState combinedState) {
- this.name = requireNonNull(pluginName);
+ public ChangeCheckInfo(CombinedCheckState combinedState) {
this.combinedState = requireNonNull(combinedState);
}
+ public ChangeCheckInfo(String pluginName, CombinedCheckState combinedState) {
+ this(combinedState);
+ this.name = requireNonNull(pluginName);
+ }
+
@Override
public boolean equals(Object o) {
if (!(o instanceof ChangeCheckInfo)) {
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
index 85d36e7..980a1fe 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
@@ -30,6 +30,7 @@
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.ChecksUpdate;
+import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
@@ -74,6 +75,7 @@
private final ChangeNoteUtil noteUtil;
private final Optional<IdentifiedUser> currentUser;
private final Checkers checkers;
+ private final CombinedCheckStateCache combinedCheckStateCache;
@AssistedInject
NoteDbChecksUpdate(
@@ -82,9 +84,17 @@
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
Checkers checkers,
+ CombinedCheckStateCache combinedCheckStateCache,
@GerritPersonIdent PersonIdent personIdent) {
this(
- repoManager, gitRefUpdated, retryHelper, noteUtil, checkers, personIdent, Optional.empty());
+ repoManager,
+ gitRefUpdated,
+ retryHelper,
+ noteUtil,
+ checkers,
+ combinedCheckStateCache,
+ personIdent,
+ Optional.empty());
}
@AssistedInject
@@ -94,6 +104,7 @@
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
Checkers checkers,
+ CombinedCheckStateCache combinedCheckStateCache,
@GerritPersonIdent PersonIdent personIdent,
@Assisted IdentifiedUser currentUser) {
this(
@@ -102,6 +113,7 @@
retryHelper,
noteUtil,
checkers,
+ combinedCheckStateCache,
personIdent,
Optional.of(currentUser));
}
@@ -112,6 +124,7 @@
RetryHelper retryHelper,
ChangeNoteUtil noteUtil,
Checkers checkers,
+ CombinedCheckStateCache combinedCheckStateCache,
@GerritPersonIdent PersonIdent personIdent,
Optional<IdentifiedUser> currentUser) {
this.repoManager = repoManager;
@@ -121,6 +134,7 @@
this.checkers = checkers;
this.currentUser = currentUser;
this.personIdent = personIdent;
+ this.combinedCheckStateCache = combinedCheckStateCache;
}
@Override
@@ -193,6 +207,7 @@
refUpdate.update();
RefUpdateUtil.checkResult(refUpdate);
+ combinedCheckStateCache.updateIfNecessary(checkKey.repository(), checkKey.patchSet());
gitRefUpdated.fire(
checkKey.repository(), refUpdate, currentUser.map(user -> user.state()).orElse(null));
return readSingleCheck(checkKey, repo, rw, newCommitId);
diff --git a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
index 5a8cbf6..b1bfd63 100644
--- a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
+++ b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
@@ -23,7 +23,7 @@
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -33,7 +33,6 @@
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.io.IOException;
import java.util.Collection;
@Singleton
@@ -55,11 +54,11 @@
}
}
- private final Checks checks;
+ private final CombinedCheckStateCache combinedCheckStateCache;
@Inject
- public ChecksSubmitRule(Checks checks) {
- this.checks = checks;
+ public ChecksSubmitRule(CombinedCheckStateCache combinedCheckStateCache) {
+ this.combinedCheckStateCache = combinedCheckStateCache;
}
@Override
@@ -79,8 +78,9 @@
CombinedCheckState combinedCheckState;
try {
- combinedCheckState = checks.getCombinedCheckState(project, currentPathSetId);
- } catch (IOException | StorageException e) {
+ // Reload value in cache to fix up inconsistencies between cache and actual state.
+ combinedCheckState = combinedCheckStateCache.reload(project, currentPathSetId);
+ } catch (StorageException e) {
String errorMessage =
String.format("failed to evaluate check states for change %s", changeId);
logger.atSevere().withCause(e).log(errorMessage);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
index 9c33099..d8b32d7 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
@@ -10,6 +10,7 @@
deps = [
"//java/com/google/gerrit/git/testing",
"//java/com/google/gerrit/server/util/time",
+ "//java/com/google/gerrit/truth",
"//javatests/com/google/gerrit/acceptance/rest/util",
"//plugins/checks:checks__plugin",
"//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
index e95fce3..d153f51 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
@@ -18,35 +18,43 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.plugins.checks.api.BlockingCondition.STATE_NOT_PASSING;
+import static com.google.gerrit.truth.CacheStatsSubject.assertThat;
+import static com.google.gerrit.truth.CacheStatsSubject.cloneStats;
+import com.google.common.cache.CacheStats;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.ChangeCheckInfo;
+import com.google.gerrit.plugins.checks.api.CheckInput;
import com.google.gerrit.plugins.checks.api.CheckState;
import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
+import java.util.List;
import java.util.Optional;
import org.junit.Before;
import org.junit.Test;
public class ChangeCheckInfoIT extends AbstractCheckersTest {
+ private CombinedCheckStateCache cache;
private Change.Id changeId;
private PatchSet.Id psId;
@Before
public void setUp() throws Exception {
+ cache = plugin.getSysInjector().getInstance(CombinedCheckStateCache.class);
psId = createChange().getPatchSetId();
changeId = psId.changeId();
}
@Test
- public void infoRequiresOption() throws Exception {
+ public void infoRequiresOptionViaGet() throws Exception {
assertThat(
getChangeCheckInfo(gApi.changes().id(changeId.get()).get(ImmutableListMultimap.of())))
.isEmpty();
@@ -55,14 +63,25 @@
}
@Test
- public void noCheckers() throws Exception {
- assertThat(checkerOperations.checkersOf(project)).isEmpty();
- assertThat(getChangeCheckInfo(changeId))
+ public void infoRequiresOptionViaQuery() throws Exception {
+ List<ChangeInfo> changeInfos = gApi.changes().query("change:" + changeId).get();
+ assertThat(changeInfos).hasSize(1);
+ assertThat(getChangeCheckInfo(changeInfos.get(0))).isEmpty();
+ assertThat(queryChangeCheckInfo(changeId))
.hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
}
@Test
- public void backfillRelevantChecker() throws Exception {
+ public void noCheckers() throws Exception {
+ assertThat(checkerOperations.checkersOf(project)).isEmpty();
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ }
+
+ @Test
+ public void backfillRelevantCheckerViaGet() throws Exception {
checkerOperations.newChecker().repository(project).query("topic:foo").create();
assertThat(getChangeCheckInfo(changeId))
.hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
@@ -72,7 +91,7 @@
}
@Test
- public void combinedCheckState() throws Exception {
+ public void combinedCheckStateViaGet() throws Exception {
CheckerUuid optionalCheckerUuid = checkerOperations.newChecker().repository(project).create();
CheckerUuid requiredCheckerUuid =
checkerOperations
@@ -96,11 +115,135 @@
.hasValue(new ChangeCheckInfo("checks", CombinedCheckState.FAILED));
}
+ @Test
+ public void combinedCheckStateViaQuery() throws Exception {
+ CacheStats start = cloneStats(cache.getStats());
+ long startReloadsFalse = cache.getReloadCount(false);
+ long startReloadsTrue = cache.getReloadCount(true);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ // Cache was prepopulated during update.
+ assertThat(cache.getStats()).since(start).hasHitCount(1);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ assertThat(cache.getStats()).since(start).hasHitCount(2);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+ }
+
+ @Test
+ public void loadingCombinedCheckStateViaGetUpdatesCache() throws Exception {
+ cache.putForTest(project, psId, CombinedCheckState.FAILED);
+
+ CacheStats start = cloneStats(cache.getStats());
+ long startReloadsFalse = cache.getReloadCount(false);
+ long startReloadsTrue = cache.getReloadCount(true);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.FAILED));
+ assertThat(cache.getStats()).since(start).hasHitCount(1);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ // Incurs reloads in both submit rule and attribute factory paths.
+ assertThat(cache.getStats()).since(start).hasHitCount(3);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(1);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ assertThat(cache.getStats()).since(start).hasHitCount(4);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(1);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+ }
+
+ @Test
+ public void updatingCheckStateUpdatesCache() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ cache.putForTest(project, psId, CombinedCheckState.IN_PROGRESS);
+
+ CacheStats start = cloneStats(cache.getStats());
+ long startReloadsFalse = cache.getReloadCount(false);
+ long startReloadsTrue = cache.getReloadCount(true);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.IN_PROGRESS));
+ assertThat(cache.getStats()).since(start).hasHitCount(1);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+
+ // Set non-required checker to FAILED, updating combined check state to WARNING.
+ CheckInput checkInput = new CheckInput();
+ checkInput.state = CheckState.FAILED;
+ checksApiFactory.revision(psId).id(checkerUuid).update(checkInput);
+
+ // Incurs reload after updating check state.
+ assertThat(cache.getStats()).since(start).hasHitCount(2);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+
+ assertThat(queryChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.WARNING));
+ assertThat(cache.getStats()).since(start).hasHitCount(3);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+ }
+
+ @Test
+ public void repeatedlyLoadingCombinedCheckStateViaGetResultsInOnlyNoOpReloads() throws Exception {
+ CacheStats start = cloneStats(cache.getStats());
+ long startReloadsFalse = cache.getReloadCount(false);
+ long startReloadsTrue = cache.getReloadCount(true);
+
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ // Incurs reloads in both submit rule and attribute factory paths.
+ assertThat(cache.getStats()).since(start).hasHitCount(2);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(2);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ assertThat(cache.getStats()).since(start).hasHitCount(4);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(4);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+
+ assertThat(getChangeCheckInfo(changeId))
+ .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+ assertThat(cache.getStats()).since(start).hasHitCount(6);
+ assertThat(cache.getStats()).since(start).hasMissCount(0);
+ assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(6);
+ assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+ }
+
private Optional<ChangeCheckInfo> getChangeCheckInfo(Change.Id id) throws Exception {
return getChangeCheckInfo(
gApi.changes().id(id.get()).get(ImmutableListMultimap.of("checks--combined", "true")));
}
+ private Optional<ChangeCheckInfo> queryChangeCheckInfo(Change.Id id) throws Exception {
+ List<ChangeInfo> changeInfos =
+ gApi.changes().query("change:" + id).withPluginOption("checks--combined", "true").get();
+ assertThat(changeInfos).hasSize(1);
+ return getChangeCheckInfo(changeInfos.get(0));
+ }
+
private static Optional<ChangeCheckInfo> getChangeCheckInfo(ChangeInfo changeInfo) {
if (changeInfo.plugins == null) {
return Optional.empty();
diff --git a/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
index f892b37..4a3bab7 100644
--- a/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
@@ -22,7 +22,7 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
@@ -38,7 +38,7 @@
@Test
public void loadingCurrentPatchSetFails() throws Exception {
ChecksSubmitRule checksSubmitRule =
- new ChecksSubmitRule(EasyMock.createStrictMock(Checks.class));
+ new ChecksSubmitRule(EasyMock.createStrictMock(CombinedCheckStateCache.class));
ChangeData cd = EasyMock.createStrictMock(ChangeData.class);
expect(cd.project()).andReturn(Project.nameKey("My-Project"));
@@ -53,12 +53,11 @@
@Test
public void getCombinedCheckStateFails() throws Exception {
- Checks checks = EasyMock.createStrictMock(Checks.class);
- expect(checks.getCombinedCheckState(anyObject(), anyObject()))
- .andThrow(new StorageException("Fail for test"));
- replay(checks);
+ CombinedCheckStateCache cache = EasyMock.createStrictMock(CombinedCheckStateCache.class);
+ expect(cache.reload(anyObject(), anyObject())).andThrow(new StorageException("Fail for test"));
+ replay(cache);
- ChecksSubmitRule checksSubmitRule = new ChecksSubmitRule(checks);
+ ChecksSubmitRule checksSubmitRule = new ChecksSubmitRule(cache);
Change.Id changeId = Change.id(1);
ChangeData cd = EasyMock.createStrictMock(ChangeData.class);
diff --git a/proto/BUILD b/proto/BUILD
new file mode 100644
index 0000000..6e3b839
--- /dev/null
+++ b/proto/BUILD
@@ -0,0 +1,12 @@
+package(default_visibility = ["//plugins/checks:visibility"])
+
+proto_library(
+ name = "cache_proto",
+ srcs = ["cache.proto"],
+)
+
+java_proto_library(
+ name = "cache_java_proto",
+ visibility = ["//visibility:public"],
+ deps = [":cache_proto"],
+)
diff --git a/proto/cache.proto b/proto/cache.proto
new file mode 100644
index 0000000..e658918
--- /dev/null
+++ b/proto/cache.proto
@@ -0,0 +1,32 @@
+// Copyright (C) 2019 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.
+
+syntax = "proto3";
+
+package gerrit.plugins.checks.cache;
+
+option java_package = "com.google.gerrit.plugins.checks.cache.proto";
+
+// Cache key for CombinedCheckStateCache.
+// Next ID: 4
+message CombinedCheckStateCacheKeyProto {
+ // Project name for the change.
+ string project = 1;
+
+ // Change number for the change.
+ int32 change_id = 2;
+
+ // Patch set to get combined state for.
+ int32 patch_set_id = 3;
+}
