Merge changes I257382ad,I8ce16c97
* changes:
ChecksSubmitRule: Catch all RuntimeExceptions
Populate CombinedCheckState in query path from a cache
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..3d10ef7 100644
--- a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
+++ b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
@@ -20,10 +20,9 @@
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitRecord.Status;
import com.google.gerrit.common.data.SubmitRequirement;
-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 +32,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 +53,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
@@ -70,7 +68,7 @@
PatchSet.Id currentPathSetId;
try {
currentPathSetId = changeData.currentPatchSet().getId();
- } catch (StorageException e) {
+ } catch (RuntimeException e) {
String errorMessage =
String.format("failed to load the current patch set of change %s", changeId);
logger.atSevere().withCause(e).log(errorMessage);
@@ -79,8 +77,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 (RuntimeException 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..7101a0f 100644
--- a/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
@@ -21,8 +21,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,12 +37,12 @@
@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"));
expect(cd.getId()).andReturn(Change.id(1));
- expect(cd.currentPatchSet()).andThrow(new StorageException("Fail for test"));
+ expect(cd.currentPatchSet()).andThrow(new IllegalStateException("Fail for test"));
replay(cd);
Collection<SubmitRecord> submitRecords =
@@ -53,12 +52,12 @@
@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 IllegalStateException("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;
+}
