Return better response when repo is out of sync
Returning LOCK_FAILURE as Result when the node is out of sync
with the shared ref-db so the client can retry if needed.
Feature: Issue 12129
Change-Id: If1a6f5e035f17c0876d0bb6f4069201099682bb5
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
index 932f0c4..cfe585d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidator.java
@@ -19,6 +19,7 @@
import com.google.inject.assistedinject.Assisted;
import com.googlesource.gerrit.plugins.multisite.LockWrapper;
import com.googlesource.gerrit.plugins.multisite.SharedRefDatabaseWrapper;
+import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.OutOfSyncException;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement.EnforcePolicy;
import java.io.IOException;
@@ -100,6 +101,13 @@
refsToUpdate = compareAndGetLatestLocalRefs(refsToUpdate, locks);
delegateUpdate.invoke();
updateSharedRefDb(batchRefUpdate.getCommands().stream(), refsToUpdate);
+ } catch (OutOfSyncException e) {
+ List<ReceiveCommand> receiveCommands = batchRefUpdate.getCommands();
+ logger.atWarning().withCause(e).log(
+ String.format(
+ "Batch ref-update failing because node is out of sync with the shared ref-db. Set all commands Result to LOCK_FAILURE [%d]",
+ receiveCommands.size()));
+ receiveCommands.forEach((command) -> command.setResult(ReceiveCommand.Result.LOCK_FAILURE));
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
index a63c00b..1f87b77 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidator.java
@@ -127,6 +127,11 @@
updateSharedDbOrThrowExceptionFor(refPairForUpdate);
}
return result;
+ } catch (OutOfSyncException e) {
+ logger.atWarning().withCause(e).log(
+ String.format("Local node is out of sync with ref-db: %s", e.getMessage()));
+
+ return RefUpdate.Result.LOCK_FAILURE;
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidatorTest.java
index 133c35d..0960cf1 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/BatchRefUpdateValidatorTest.java
@@ -14,8 +14,11 @@
package com.googlesource.gerrit.plugins.multisite.validation;
+import static com.google.common.truth.Truth.assertThat;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.UPDATE;
import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@@ -27,6 +30,7 @@
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedRefEnforcement;
import java.io.IOException;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import org.eclipse.jgit.internal.storage.file.RefDirectory;
import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
@@ -59,6 +63,8 @@
@Mock SharedRefDatabaseWrapper sharedRefDatabase;
+ @Mock SharedRefEnforcement tmpRefEnforcement;
+
@Before
public void setup() throws Exception {
super.setUp();
@@ -105,6 +111,32 @@
.compareAndPut(A_TEST_PROJECT_NAME_KEY, newRef(DRAFT_COMMENT, A.getId()), B.getId());
}
+ @Test
+ public void validationShouldFailWhenLocalRefDbIsOutOfSync() throws Exception {
+ String AN_OUT_OF_SYNC_REF = "refs/changes/01/1/1";
+ BatchRefUpdate batchRefUpdate =
+ newBatchUpdate(
+ Collections.singletonList(new ReceiveCommand(A, B, AN_OUT_OF_SYNC_REF, UPDATE)));
+ BatchRefUpdateValidator batchRefUpdateValidator =
+ getRefValidatorForEnforcement(A_TEST_PROJECT_NAME, tmpRefEnforcement);
+
+ doReturn(SharedRefEnforcement.EnforcePolicy.REQUIRED)
+ .when(batchRefUpdateValidator.refEnforcement)
+ .getPolicy(A_TEST_PROJECT_NAME, AN_OUT_OF_SYNC_REF);
+ lenient()
+ .doReturn(false)
+ .when(sharedRefDatabase)
+ .isUpToDate(A_TEST_PROJECT_NAME_KEY, newRef(AN_OUT_OF_SYNC_REF, AN_OBJECT_ID_1));
+
+ batchRefUpdateValidator.executeBatchUpdateWithValidation(
+ batchRefUpdate, () -> execute(batchRefUpdate));
+
+ final List<ReceiveCommand> commands = batchRefUpdate.getCommands();
+ assertThat(commands.size()).isEqualTo(1);
+ commands.forEach(
+ (command) -> assertThat(command.getResult()).isEqualTo(ReceiveCommand.Result.LOCK_FAILURE));
+ }
+
private BatchRefUpdateValidator newDefaultValidator(String projectName) {
return getRefValidatorForEnforcement(projectName, new DefaultSharedRefEnforcement());
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
index f6eadcc..187f686 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/MultiSiteBatchRefUpdateTest.java
@@ -15,10 +15,11 @@
package com.googlesource.gerrit.plugins.multisite.validation;
import static java.util.Arrays.asList;
-import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
@@ -50,6 +51,7 @@
@Mock SharedRefDatabaseWrapper sharedRefDb;
@Mock BatchRefUpdate batchRefUpdate;
+ @Mock BatchRefUpdateValidator batchRefUpdateValidator;
@Mock RefDatabase refDatabase;
@Mock RevWalk revWalk;
@Mock ProgressMonitor progressMonitor;
@@ -123,17 +125,25 @@
return argThat(new RefMatcher(oldRef));
}
- @Test
+ @Test(expected = IOException.class)
public void executeAndFailsWithExceptions() throws IOException {
+ multiSiteRefUpdate = getMultiSiteBatchRefUpdateWithMockedValidator();
+ doThrow(new IOException("IO Test Exception"))
+ .when(batchRefUpdateValidator)
+ .executeBatchUpdateWithValidation(any(), any());
+
+ multiSiteRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
+ }
+
+ @Test
+ public void executeSuccessfullyWithNoExceptionsWhenOutOfSync() throws IOException {
setMockRequiredReturnValues();
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);
- try {
- multiSiteRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
- fail("Expecting an IOException to be thrown");
- } catch (IOException e) {
- verify(validationMetrics).incrementSplitBrainPrevention();
- }
+
+ multiSiteRefUpdate.execute(revWalk, progressMonitor, Collections.emptyList());
+
+ verify(validationMetrics).incrementSplitBrainPrevention();
}
@Test
@@ -163,6 +173,17 @@
return new MultiSiteBatchRefUpdate(batchRefValidatorFactory, A_TEST_PROJECT_NAME, refDatabase);
}
+ private MultiSiteBatchRefUpdate getMultiSiteBatchRefUpdateWithMockedValidator() {
+ BatchRefUpdateValidator.Factory batchRefValidatorFactory =
+ new BatchRefUpdateValidator.Factory() {
+ @Override
+ public BatchRefUpdateValidator create(String projectName, RefDatabase refDb) {
+ return batchRefUpdateValidator;
+ }
+ };
+ return new MultiSiteBatchRefUpdate(batchRefValidatorFactory, A_TEST_PROJECT_NAME, refDatabase);
+ }
+
protected static class RefMatcher implements ArgumentMatcher<Ref> {
private Ref left;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
index 3887611..a9968c4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/RefUpdateValidatorTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.reviewdb.client.Project;
import com.googlesource.gerrit.plugins.multisite.SharedRefDatabaseWrapper;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.DefaultSharedRefEnforcement;
-import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.OutOfSyncException;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.RefFixture;
import com.googlesource.gerrit.plugins.multisite.validation.dfsrefdb.SharedDbSplitBrainException;
import org.eclipse.jgit.lib.ObjectId;
@@ -139,8 +138,8 @@
assertThat(result).isEqualTo(RefUpdate.Result.NEW);
}
- @Test(expected = OutOfSyncException.class)
- public void validationShouldFailWhenLocalRefDbIsNotUpToDate() throws Exception {
+ @Test
+ public void validationShouldFailWhenLocalRefDbIsOutOfSync() throws Exception {
lenient()
.doReturn(true)
.when(sharedRefDb)
@@ -148,7 +147,9 @@
doReturn(true).when(sharedRefDb).exists(A_TEST_PROJECT_NAME_KEY, refName);
doReturn(false).when(sharedRefDb).isUpToDate(A_TEST_PROJECT_NAME_KEY, localRef);
- refUpdateValidator.executeRefUpdate(refUpdate, () -> RefUpdate.Result.NEW);
+ Result result = refUpdateValidator.executeRefUpdate(refUpdate, () -> RefUpdate.Result.NEW);
+
+ assertThat(result).isEqualTo(Result.LOCK_FAILURE);
}
@Test(expected = SharedDbSplitBrainException.class)
