Merge branch 'stable-3.11'
* stable-3.11:
Add sentinel to prevent out of sync with global-refdb
Improve ref-update validators tests
Reformat with GJF 1.24.0
Change-Id: I98561ea5a4ec3c621bec484487262004988e3e7d
diff --git a/metrics.md b/metrics.md
index 4e53f30..0df0c24 100644
--- a/metrics.md
+++ b/metrics.md
@@ -17,4 +17,7 @@
: the latency in milliseconds of the isUpToDate operation.
* global_refdb/remove_latency
- : the latency in milliseconds of the remove operation.
\ No newline at end of file
+ : the latency in milliseconds of the remove operation.
+
+* global_refdb/operation_failures
+ : cumulative number of failures when attempting to perform an operation on global-refdb.
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDBMetrics.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDBMetrics.java
index c907831..9100a6e 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDBMetrics.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDBMetrics.java
@@ -14,6 +14,7 @@
package com.gerritforge.gerrit.globalrefdb.validation;
+import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer0;
@@ -27,6 +28,7 @@
private final Timer0 lockRefExecutionTime;
private final Timer0 getOperationExecutionTime;
private final Timer0 existsExecutionTime;
+ private final Counter0 operationFailures;
private Timer0 compareAndPutExecutionTime;
private Timer0 setExecutionTime;
private Timer0 removeExecutionTime;
@@ -77,6 +79,13 @@
new Description("Time spent on checking in global ref-db if ref is up-to-date.")
.setCumulative()
.setUnit(Description.Units.MILLISECONDS));
+ operationFailures =
+ metricMaker.newCounter(
+ "global_refdb/operation_failures",
+ new Description(
+ "Number of failures when attempting to perform an operation on global-refdb.")
+ .setCumulative()
+ .setUnit("failures"));
}
public Context startCompareAndPutExecutionTime() {
@@ -106,4 +115,8 @@
public Context startIsUpToDateExecutionTime() {
return isUpToDateExecutionTime.start();
}
+
+ public void incrementOperationFailures() {
+ operationFailures.increment();
+ }
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
index b973602..93427a7 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
@@ -20,6 +20,7 @@
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.NoopSharedRefDatabase;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Supplier;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -69,35 +70,40 @@
@Override
public boolean isUpToDate(Project.NameKey project, Ref ref) throws GlobalRefDbLockException {
- try (Context context = metrics.startIsUpToDateExecutionTime()) {
- return sharedRefDb().isUpToDate(project, ref);
- }
+ return trackFailingOperation(
+ () -> sharedRefDb().isUpToDate(project, ref), metrics::startIsUpToDateExecutionTime);
}
/** {@inheritDoc}. The operation is logged upon success. */
@Override
public boolean compareAndPut(Project.NameKey project, Ref currRef, ObjectId newRefValue)
throws GlobalRefDbSystemError {
- try (Context context = metrics.startCompareAndPutExecutionTime()) {
- boolean succeeded = sharedRefDb().compareAndPut(project, currRef, newRefValue);
- if (succeeded) {
- sharedRefLogger.logRefUpdate(project.get(), currRef, newRefValue);
- }
- return succeeded;
- }
+
+ return trackFailingOperation(
+ () -> {
+ boolean succeeded = sharedRefDb().compareAndPut(project, currRef, newRefValue);
+ if (succeeded) {
+ sharedRefLogger.logRefUpdate(project.get(), currRef, newRefValue);
+ }
+ return succeeded;
+ },
+ metrics::startCompareAndPutExecutionTime);
}
/** {@inheritDoc} the operation is logged upon success. */
@Override
public <T> boolean compareAndPut(Project.NameKey project, String refName, T currValue, T newValue)
throws GlobalRefDbSystemError {
- try (Context context = metrics.startCompareAndPutExecutionTime()) {
- boolean succeeded = sharedRefDb().compareAndPut(project, refName, currValue, newValue);
- if (succeeded) {
- sharedRefLogger.logRefUpdate(project.get(), refName, currValue, newValue);
- }
- return succeeded;
- }
+
+ return trackFailingOperation(
+ () -> {
+ boolean succeeded = sharedRefDb().compareAndPut(project, refName, currValue, newValue);
+ if (succeeded) {
+ sharedRefLogger.logRefUpdate(project.get(), refName, currValue, newValue);
+ }
+ return succeeded;
+ },
+ metrics::startCompareAndPutExecutionTime);
}
@Override
@@ -107,10 +113,14 @@
throw new UnsupportedOperationException(
"GlobalRefDb implementation doesn't support set operation");
}
- try (Context context = metrics.startSetExecutionTime()) {
- ((ExtendedGlobalRefDatabase) sharedRefDb()).put(project, refName, newValue);
- sharedRefLogger.logRefUpdate(project.get(), refName, newValue);
- }
+
+ trackFailingOperation(
+ () -> {
+ ((ExtendedGlobalRefDatabase) sharedRefDb()).put(project, refName, newValue);
+ sharedRefLogger.logRefUpdate(project.get(), refName, newValue);
+ return null;
+ },
+ metrics::startSetExecutionTime);
}
public boolean isSetOperationSupported() {
@@ -121,35 +131,38 @@
@Override
public AutoCloseable lockRef(Project.NameKey project, String refName)
throws GlobalRefDbLockException {
- try (Context context = metrics.startLockRefExecutionTime()) {
- AutoCloseable locker = sharedRefDb().lockRef(project, refName);
- sharedRefLogger.logLockAcquisition(project.get(), refName);
- return locker;
- }
+ return trackFailingOperation(
+ () -> {
+ AutoCloseable locker = sharedRefDb().lockRef(project, refName);
+ sharedRefLogger.logLockAcquisition(project.get(), refName);
+ return locker;
+ },
+ metrics::startLockRefExecutionTime);
}
@Override
public boolean exists(Project.NameKey project, String refName) {
- try (Context context = metrics.startExistsExecutionTime()) {
- return sharedRefDb().exists(project, refName);
- }
+ return trackFailingOperation(
+ () -> sharedRefDb().exists(project, refName), metrics::startExistsExecutionTime);
}
/** {@inheritDoc}. The operation is logged. */
@Override
public void remove(Project.NameKey project) throws GlobalRefDbSystemError {
- try (Context context = metrics.startRemoveExecutionTime()) {
- sharedRefDb().remove(project);
- sharedRefLogger.logProjectDelete(project.get());
- }
+ trackFailingOperation(
+ () -> {
+ sharedRefDb().remove(project);
+ sharedRefLogger.logProjectDelete(project.get());
+ return null;
+ },
+ metrics::startRemoveExecutionTime);
}
@Override
public <T> Optional<T> get(Project.NameKey nameKey, String s, Class<T> clazz)
throws GlobalRefDbSystemError {
- try (Context context = metrics.startGetExecutionTime()) {
- return sharedRefDb().get(nameKey, s, clazz);
- }
+ return trackFailingOperation(
+ () -> sharedRefDb().get(nameKey, s, clazz), metrics::startGetExecutionTime);
}
boolean isNoop() {
@@ -169,4 +182,24 @@
return NOOP_REFDB;
});
}
+
+ @FunctionalInterface
+ private interface ThrowingSupplier<T, E extends Exception> {
+ T get() throws E;
+ }
+
+ private <T, E extends Exception> T trackFailingOperation(
+ ThrowingSupplier<T, E> operation, Supplier<Context> metricTimer) throws E {
+
+ boolean completedWithoutExceptions = false;
+ try (Context ignore = metricTimer.get()) {
+ T result = operation.get();
+ completedWithoutExceptions = true;
+ return result;
+ } finally {
+ if (!completedWithoutExceptions) {
+ metrics.incrementOperationFailures();
+ }
+ }
+ }
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbExceptionHook.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbExceptionHook.java
index a7f2740..e0495d5 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbExceptionHook.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbExceptionHook.java
@@ -16,7 +16,7 @@
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException;
import com.google.common.collect.ImmutableList;
-import com.google.gerrit.common.Nullable;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.ExceptionHook;
import java.util.Optional;
@@ -36,12 +36,14 @@
}
@Override
- public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
+ public ImmutableList<String> getUserMessages(Throwable throwable, ImmutableSet<String> traceIds) {
if (throwable instanceof GlobalRefDbLockException) {
ImmutableList.Builder<String> builder = new ImmutableList.Builder<>();
builder.add(throwable.getMessage());
- if (traceId != null && !traceId.isBlank()) {
- builder.add(String.format("Trace ID: %s", traceId));
+ for (String traceId : traceIds) {
+ if (traceId != null && !traceId.isBlank()) {
+ builder.add(String.format("Trace ID: %s", traceId));
+ }
}
return builder.build();
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbRefDatabase.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbRefDatabase.java
index 8b51beb..6085627 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbRefDatabase.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbRefDatabase.java
@@ -26,6 +26,7 @@
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.RefRename;
import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.ReflogReader;
/**
* Wraps an instance of {@link RefDatabase} with the intent of wrapping {@link RefUpdate} operations
@@ -170,6 +171,11 @@
}
@Override
+ public ReflogReader getReflogReader(Ref ref) throws IOException {
+ return refDatabase.getReflogReader(ref);
+ }
+
+ @Override
public boolean hasRefs() throws IOException {
return refDatabase.hasRefs();
}
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java
index f26f749..6fc39bd 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java
@@ -15,6 +15,7 @@
package com.gerritforge.gerrit.globalrefdb.validation;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
@@ -236,6 +237,7 @@
Result result =
refUpdateValidator.executeRefUpdate(
refUpdate, () -> doLocalRefUpdate(localRef.getName()), rollbackFunction);
+ assertEquals(Result.LOCK_FAILURE, result);
verify(rollbackFunction).invoke(any());
}
@@ -263,7 +265,7 @@
.compareAndPut(any(Project.NameKey.class), any(Ref.class), any(ObjectId.class));
}
- private Result defaultRollback(ObjectId objectId) {
+ private Result defaultRollback(ObjectId unused) {
return Result.NO_CHANGE;
}
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java
index 5240f2c..292fa19 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java
@@ -14,11 +14,16 @@
package com.gerritforge.gerrit.globalrefdb.validation;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.gerritforge.gerrit.globalrefdb.GlobalRefDatabase;
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException;
+import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError;
+import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.NoopSharedRefDatabase;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.metrics.Timer0.Context;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -94,4 +99,29 @@
verify(metrics).startRemoveExecutionTime();
verify(context).close();
}
+
+ @Test
+ public void shouldIncreaseNumberOfFailuresWhenCompareAndPutThrows() throws Exception {
+ DynamicItem<GlobalRefDatabase> couldNotConnectGlobalRefDB =
+ DynamicItem.itemOf(
+ GlobalRefDatabase.class,
+ new NoopSharedRefDatabase() {
+ @Override
+ public boolean compareAndPut(
+ Project.NameKey project, Ref currRef, ObjectId newRefValue)
+ throws GlobalRefDbSystemError {
+ throw new GlobalRefDbSystemError(
+ "Could not write to global-refdb", new Exception("Could not connect"));
+ }
+ });
+ objectUnderTest =
+ new SharedRefDatabaseWrapper(
+ couldNotConnectGlobalRefDB, new DisabledSharedRefLogger(), metrics);
+
+ assertThrows(
+ GlobalRefDbSystemError.class,
+ () -> objectUnderTest.compareAndPut(projectName, ref, ObjectId.zeroId()));
+
+ verify(metrics).incrementOperationFailures();
+ }
}
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbBatchRefUpdateTest.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbBatchRefUpdateTest.java
index 26761f1..32a552a 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbBatchRefUpdateTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDbBatchRefUpdateTest.java
@@ -31,6 +31,7 @@
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.RefFixture;
import com.google.common.collect.ImmutableSet;
import java.io.IOException;
+import java.util.Collections;
import java.util.List;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
@@ -101,7 +102,6 @@
when(projectsFilter.matches(anyString())).thenReturn(true);
}
- @SuppressWarnings("deprecation")
private void setMockRequiredReturnValues() throws IOException {
doReturn(batchRefUpdate).when(refDatabase).newBatchUpdate();
@@ -114,7 +114,6 @@
// and not supposed to be used if all tests are successful.
lenient().when(batchRefUpdate.addCommand(any(List.class))).thenReturn(batchRefUpdate);
- lenient().when(refDatabase.getRef(A_TEST_REF_NAME)).thenReturn(oldRef).thenReturn(newRef);
lenient().when(refDatabase.exactRef(A_TEST_REF_NAME)).thenReturn(oldRef).thenReturn(newRef);
lenient().when(refDatabase.findRef(A_TEST_REF_NAME)).thenReturn(oldRef).thenReturn(newRef);
@@ -132,7 +131,7 @@
doReturn(true)
.when(sharedRefDb)
.compareAndPut(eq(A_TEST_PROJECT_NAME_KEY), refEquals(oldRef), eq(newRef.getObjectId()));
- sharedRefDbRefUpdate.execute(revWalk, progressMonitor, EMPTY_LIST);
+ sharedRefDbRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
verify(sharedRefDb)
.compareAndPut(eq(A_TEST_PROJECT_NAME_KEY), refEquals(oldRef), eq(newRef.getObjectId()));
}
@@ -148,7 +147,7 @@
.when(batchRefUpdateValidator)
.executeBatchUpdateWithValidation(any(), any(), any());
- sharedRefDbRefUpdate.execute(revWalk, progressMonitor, EMPTY_LIST);
+ sharedRefDbRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
}
@Test
@@ -157,7 +156,7 @@
doReturn(true).when(sharedRefDb).exists(A_TEST_PROJECT_NAME_KEY, A_TEST_REF_NAME);
doReturn(false).when(sharedRefDb).isUpToDate(A_TEST_PROJECT_NAME_KEY, oldRef);
- sharedRefDbRefUpdate.execute(revWalk, progressMonitor, EMPTY_LIST);
+ sharedRefDbRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
verify(validationMetrics).incrementSplitBrainPrevention();
}
@@ -169,7 +168,7 @@
sharedRefDbRefUpdate = getSharedRefDbBatchRefUpdateWithDefaultPolicyEnforcement();
- sharedRefDbRefUpdate.execute(revWalk, progressMonitor, EMPTY_LIST);
+ sharedRefDbRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
}
private SharedRefDbBatchRefUpdate getSharedRefDbBatchRefUpdateWithDefaultPolicyEnforcement() {
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/ValidationModuleTest.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/ValidationModuleTest.java
index b1af798..d54719d 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/ValidationModuleTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/ValidationModuleTest.java
@@ -14,8 +14,6 @@
package com.gerritforge.gerrit.globalrefdb.validation;
-import static java.util.Collections.EMPTY_SET;
-
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.DefaultSharedRefEnforcement;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.SharedRefEnforcement;
import com.google.common.collect.ImmutableSet;
@@ -25,6 +23,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.*;
import com.google.inject.name.Names;
+import java.util.Collections;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.Config;
@@ -61,7 +60,7 @@
public static class ValidationModuleWithEmptyIgnoredRefs extends ValidationModule {
@Inject
public ValidationModuleWithEmptyIgnoredRefs(@GerritServerConfig Config config) {
- super(config, Optional.of(EMPTY_SET));
+ super(config, Optional.of(Collections.emptySet()));
}
}
