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())); } }