Merge branch 'stable-3.11' into stable-3.12
* stable-3.11:
Fix rollback implementation
Change-Id: I9a67583e25c36df84a80eb54d427a602c1995255
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 fd5d546..6cc3be1 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
@@ -21,13 +21,16 @@
import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
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;
import com.google.gerrit.metrics.Timer0.Context;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.Arrays;
import java.util.Optional;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -74,35 +77,63 @@
@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,
+ () ->
+ toString(project, project::get) + ":" + toString(ref, ref::getName) + " is up-to-date");
}
/** {@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,
+ () ->
+ "compare "
+ + toString(project, project::get)
+ + ":"
+ + toString(currRef, currRef::getName)
+ + ":"
+ + toString(
+ currRef,
+ () -> toString(currRef.getObjectId(), () -> currRef.getObjectId().name()))
+ + " and put "
+ + toString(newRefValue, newRefValue::name));
}
/** {@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,
+ () ->
+ "compare "
+ + toString(project, project::get)
+ + ":"
+ + toString(refName)
+ + ":"
+ + toString(currValue)
+ + " and put "
+ + toString(newValue));
}
@Override
@@ -112,10 +143,21 @@
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,
+ () ->
+ "Put "
+ + toString(project, project::get)
+ + ":"
+ + toString(refName)
+ + " = "
+ + toString(newValue));
}
public boolean isSetOperationSupported() {
@@ -126,14 +168,16 @@
@Override
public AutoCloseable lockRef(Project.NameKey project, String refName)
throws GlobalRefDbLockException {
- try (Context context = metrics.startLockRefExecutionTime()) {
- return new LockWrapper(
- sharedRefLogger,
- project.get(),
- refName,
- sharedRefDb().lockRef(project, refName),
- SharedRefLogger.Scope.GLOBAL);
- }
+ return trackFailingOperation(
+ () ->
+ new LockWrapper(
+ sharedRefLogger,
+ project.get(),
+ refName,
+ sharedRefDb().lockRef(project, refName),
+ SharedRefLogger.Scope.GLOBAL),
+ metrics::startLockRefExecutionTime,
+ () -> "Lock " + toString(project, project::get) + ":" + toString(refName));
}
public AutoCloseable lockLocalRef(Project.NameKey project, String refName)
@@ -148,26 +192,38 @@
@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,
+ () -> toString(project, project::get) + ":" + toString(refName) + " exists");
}
/** {@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,
+ () -> "Remove " + toString(project, project::get));
}
@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,
+ () ->
+ "Get "
+ + toString(nameKey, nameKey::get)
+ + ":"
+ + toString(s)
+ + " of type "
+ + clazz.getSimpleName());
}
boolean isNoop() {
@@ -187,4 +243,42 @@
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,
+ Supplier<String> operationDetails)
+ throws E {
+
+ boolean completedWithoutExceptions = false;
+ try (Context ignore = metricTimer.get()) {
+ T result = operation.get();
+ completedWithoutExceptions = true;
+ return result;
+ } finally {
+ if (!completedWithoutExceptions) {
+ String callStack =
+ Arrays.stream(Thread.currentThread().getStackTrace())
+ .skip(2) /* Skip the getStackTrace() and trackFailingOperation() call stacks */
+ .map(StackTraceElement::toString)
+ .collect(Collectors.joining("\n at "));
+ log.atWarning().log(
+ "Global-refdb operation '%s' failed\n%s", operationDetails.get(), callStack);
+ metrics.incrementOperationFailures();
+ }
+ }
+ }
+
+ private static <T> String toString(T value) {
+ return String.valueOf(value);
+ }
+
+ private static <O, T> String toString(O object, Supplier<T> valueSupplier) {
+ return toString(object == null ? null : valueSupplier.get());
+ }
}
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 435db8f..01018f2 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;
@@ -249,6 +250,7 @@
Result result =
refUpdateValidator.executeRefUpdate(
refUpdate, () -> doLocalRefUpdate(localRef.getName()), rollbackFunction);
+ assertEquals(Result.LOCK_FAILURE, result);
verify(rollbackFunction).invoke(any());
}
@@ -276,7 +278,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 ce1fcea..dda7d9f 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;
@@ -95,4 +100,32 @@
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,
+ NoOpRefLocker.INSTANCE);
+
+ 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 e463cb5..4c6290d 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 3283994..1547bd3 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()));
}
}
