Introduce local-refdb locking facility
Allow to prevent local writes by locking one or more refs through the
same SharedRefDatabaseWrapper class used for the global locks.
Having local locks help preventing local mutations without having to
lock all the other nodes.
Change-Id: Ic2fca440b87d9142cb4ec7c810dcbcb2bad35242
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/GlobalRefDbLockException.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/GlobalRefDbLockException.java
index fa757db..29ac90e 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/GlobalRefDbLockException.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/GlobalRefDbLockException.java
@@ -18,7 +18,7 @@
* {@code GlobalRefDbLockException} is an exception that can be thrown when interacting with the
* global-refdb to represent the inability to lock or acquire a resource.
*/
-public class GlobalRefDbLockException extends RuntimeException {
+public class GlobalRefDbLockException extends RefDbLockException {
private static final long serialVersionUID = 1L;
/**
@@ -30,6 +30,6 @@
* @param cause the cause of the locking failure
*/
public GlobalRefDbLockException(String project, String refName, Exception cause) {
- super(String.format("Unable to lock ref %s on project %s", refName, project), cause);
+ super(project, refName, cause);
}
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/RefDbLockException.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/RefDbLockException.java
new file mode 100644
index 0000000..27ac348
--- /dev/null
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/RefDbLockException.java
@@ -0,0 +1,35 @@
+// Copyright (C) 2025 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.gerritforge.gerrit.globalrefdb;
+
+/** {@code RefDbLockException} is an exception that can be thrown when trying to lock a ref. */
+public class RefDbLockException extends RuntimeException {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * Constructs a new {@code RefDbLockException} with the specified project, refName and cause.
+ *
+ * @param project the project containing refName
+ * @param refName the specific ref for which the locking failed
+ * @param cause the cause of the locking failure
+ */
+ public RefDbLockException(String project, String refName, Exception cause) {
+ super(String.format("Unable to lock ref %s on project %s", refName, project), cause);
+ }
+
+ public RefDbLockException(String project, String refName, String message) {
+ super(String.format("Unable to lock ref %s on project %s: %s", refName, project, message));
+ }
+}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/LockWrapper.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/LockWrapper.java
index e52705f..02b2ec0 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/LockWrapper.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/LockWrapper.java
@@ -20,6 +20,7 @@
private final String refName;
private final AutoCloseable lock;
private final SharedRefLogger sharedRefLogger;
+ private final SharedRefLogger.Scope scope;
/**
* Constructs a {@code LockWrapper} object for a specific refName of a project, which wraps a held
@@ -31,12 +32,17 @@
* @param lock the acquired lock
*/
public LockWrapper(
- SharedRefLogger sharedRefLogger, String project, String refName, AutoCloseable lock) {
+ SharedRefLogger sharedRefLogger,
+ String project,
+ String refName,
+ AutoCloseable lock,
+ SharedRefLogger.Scope scope) {
this.lock = lock;
this.sharedRefLogger = sharedRefLogger;
this.project = project;
this.refName = refName;
- sharedRefLogger.logLockAcquisition(project, refName);
+ this.scope = scope;
+ sharedRefLogger.logLockAcquisition(project, refName, scope);
}
/**
@@ -47,6 +53,6 @@
@Override
public void close() throws Exception {
lock.close();
- sharedRefLogger.logLockRelease(project, refName);
+ sharedRefLogger.logLockRelease(project, refName, scope);
}
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLogger.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLogger.java
index 04ffeed..691d8b1 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLogger.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLogger.java
@@ -168,8 +168,8 @@
* <p>Logs Json serialization of {@link SharedRefLogEntry.LockAcquire}
*/
@Override
- public void logLockAcquisition(String project, String refName) {
- sharedRefDBLog.info(gson.toJson(new SharedRefLogEntry.LockAcquire(project, refName)));
+ public void logLockAcquisition(String project, String refName, Scope scope) {
+ sharedRefDBLog.info(gson.toJson(new SharedRefLogEntry.LockAcquire(project, refName, scope)));
}
/**
@@ -178,8 +178,8 @@
* <p>Logs Json serialization of {@link SharedRefLogEntry.LockRelease}
*/
@Override
- public void logLockRelease(String project, String refName) {
- sharedRefDBLog.info(gson.toJson(new SharedRefLogEntry.LockRelease(project, refName)));
+ public void logLockRelease(String project, String refName, Scope scope) {
+ sharedRefDBLog.info(gson.toJson(new SharedRefLogEntry.LockRelease(project, refName, scope)));
}
@VisibleForTesting
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/NoOpRefLocker.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/NoOpRefLocker.java
new file mode 100644
index 0000000..b8682b4
--- /dev/null
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/NoOpRefLocker.java
@@ -0,0 +1,30 @@
+// Copyright (C) 2025 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.gerritforge.gerrit.globalrefdb.validation;
+
+import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
+import com.google.gerrit.entities.Project;
+import com.google.inject.Singleton;
+
+@Singleton
+class NoOpRefLocker implements RefLocker {
+
+ public static final NoOpRefLocker INSTANCE = new NoOpRefLocker();
+
+ @Override
+ public AutoCloseable lockRef(Project.NameKey project, String refName) throws RefDbLockException {
+ return () -> {};
+ }
+}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefLocker.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefLocker.java
new file mode 100644
index 0000000..c0f1578
--- /dev/null
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefLocker.java
@@ -0,0 +1,24 @@
+// Copyright (C) 2025 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.gerritforge.gerrit.globalrefdb.validation;
+
+import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
+import com.google.gerrit.entities.Project;
+import com.google.inject.ImplementedBy;
+
+@ImplementedBy(NoOpRefLocker.class)
+public interface RefLocker {
+ AutoCloseable lockRef(Project.NameKey project, String refName) throws RefDbLockException;
+}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java
index cdeb0fa..50986da 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidator.java
@@ -15,6 +15,7 @@
package com.gerritforge.gerrit.globalrefdb.validation;
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError;
+import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.CustomSharedRefEnforcementByProject;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.DefaultSharedRefEnforcement;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.OutOfSyncException;
@@ -201,6 +202,9 @@
e.getMessage());
}
return result;
+ } catch (RefDbLockException e) {
+ logger.atWarning().withCause(e).log("Unable to lock %s:%s", projectName, refUpdate.getName());
+ return Result.LOCK_FAILURE;
} catch (OutOfSyncException e) {
logger.atWarning().withCause(e).log(
"Local node is out of sync with ref-db: %s", e.getMessage());
@@ -263,17 +267,18 @@
return refPair;
}
- locks.addResourceIfNotExist(
- String.format("%s-%s", projectName, refName),
- () -> sharedRefDb.lockRef(Project.nameKey(projectName), refName));
+ String sharedLockKey = String.format("%s:%s", projectName, refName);
+ String localLockKey = String.format("%s:local", sharedLockKey);
+ Project.NameKey projectKey = Project.nameKey(projectName);
+ locks.addResourceIfNotExist(localLockKey, () -> sharedRefDb.lockLocalRef(projectKey, refName));
+ locks.addResourceIfNotExist(sharedLockKey, () -> sharedRefDb.lockRef(projectKey, refName));
RefPair latestRefPair = getLatestLocalRef(refPair);
- if (sharedRefDb.isUpToDate(Project.nameKey(projectName), latestRefPair.compareRef)) {
+ if (sharedRefDb.isUpToDate(projectKey, latestRefPair.compareRef)) {
return latestRefPair;
}
- if (isNullRef(latestRefPair.compareRef)
- || sharedRefDb.exists(Project.nameKey(projectName), refName)) {
+ if (isNullRef(latestRefPair.compareRef) || sharedRefDb.exists(projectKey, refName)) {
validationMetrics.incrementSplitBrainPrevention();
softFailBasedOnEnforcement(
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 d89e535..fd5d546 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapper.java
@@ -18,6 +18,7 @@
import com.gerritforge.gerrit.globalrefdb.GlobalRefDatabase;
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException;
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError;
+import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.NoopSharedRefDatabase;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.flogger.FluentLogger;
@@ -45,6 +46,7 @@
private final SharedRefLogger sharedRefLogger;
private final SharedRefDBMetrics metrics;
+ private final RefLocker localRefDbLocker;
/**
* Constructs a {@code SharedRefDatabaseWrapper} wrapping an optional {@link GlobalRefDatabase},
@@ -53,17 +55,20 @@
* @param sharedRefLogger logger of shared ref-db operations.
*/
@Inject
- public SharedRefDatabaseWrapper(SharedRefLogger sharedRefLogger, SharedRefDBMetrics metrics) {
+ public SharedRefDatabaseWrapper(
+ SharedRefLogger sharedRefLogger, SharedRefDBMetrics metrics, RefLocker localRefDbLocker) {
this.sharedRefLogger = sharedRefLogger;
this.metrics = metrics;
+ this.localRefDbLocker = localRefDbLocker;
}
@VisibleForTesting
public SharedRefDatabaseWrapper(
DynamicItem<GlobalRefDatabase> sharedRefDbDynamicItem,
SharedRefLogger sharedRefLogger,
- SharedRefDBMetrics metrics) {
- this(sharedRefLogger, metrics);
+ SharedRefDBMetrics metrics,
+ RefLocker localRefDbLocker) {
+ this(sharedRefLogger, metrics, localRefDbLocker);
this.sharedRefDbDynamicItem = sharedRefDbDynamicItem;
}
@@ -123,10 +128,24 @@
throws GlobalRefDbLockException {
try (Context context = metrics.startLockRefExecutionTime()) {
return new LockWrapper(
- sharedRefLogger, project.get(), refName, sharedRefDb().lockRef(project, refName));
+ sharedRefLogger,
+ project.get(),
+ refName,
+ sharedRefDb().lockRef(project, refName),
+ SharedRefLogger.Scope.GLOBAL);
}
}
+ public AutoCloseable lockLocalRef(Project.NameKey project, String refName)
+ throws RefDbLockException {
+ return new LockWrapper(
+ sharedRefLogger,
+ project.get(),
+ refName,
+ localRefDbLocker.lockRef(project, refName),
+ SharedRefLogger.Scope.LOCAL);
+ }
+
@Override
public boolean exists(Project.NameKey project, String refName) {
try (Context context = metrics.startExistsExecutionTime()) {
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogEntry.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogEntry.java
index 4b8132f..817b274 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogEntry.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogEntry.java
@@ -79,22 +79,26 @@
public static class LockAcquire extends SharedRefLogEntry {
public String refName;
+ public SharedRefLogger.Scope scope;
- LockAcquire(String projectName, String refName) {
+ LockAcquire(String projectName, String refName, SharedRefLogger.Scope scope) {
this.type = Type.LOCK_ACQUIRE;
this.projectName = projectName;
this.refName = refName;
+ this.scope = scope;
}
}
public static class LockRelease extends SharedRefLogEntry {
public String refName;
+ public SharedRefLogger.Scope scope;
- LockRelease(String projectName, String refName) {
+ LockRelease(String projectName, String refName, SharedRefLogger.Scope scope) {
this.type = Type.LOCK_RELEASE;
this.projectName = projectName;
this.refName = refName;
+ this.scope = scope;
}
}
}
diff --git a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogger.java b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogger.java
index f61f253..96db4e4 100644
--- a/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogger.java
+++ b/src/main/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefLogger.java
@@ -21,6 +21,11 @@
@ImplementedBy(Log4jSharedRefLogger.class)
public interface SharedRefLogger {
+ enum Scope {
+ GLOBAL,
+ LOCAL
+ }
+
/**
* Log the update of currRef in project project to ref newRefValue
*
@@ -63,14 +68,16 @@
*
* @param project the project containing the ref
* @param refName the name of the ref the lock is acquired for
+ * @param scope scope of the lock
*/
- void logLockAcquisition(String project, String refName);
+ void logLockAcquisition(String project, String refName, Scope scope);
/**
* Log the releasing of a previously acquired lock for the 'refName' of 'project'
*
* @param project the project containing the ref
* @param refName the name of the ref the lock is being releaed for
+ * @param scope scope of the lock
*/
- void logLockRelease(String project, String refName);
+ void logLockRelease(String project, String refName, Scope scope);
}
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/DisabledSharedRefLogger.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/DisabledSharedRefLogger.java
index d6dd338..873f101 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/DisabledSharedRefLogger.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/DisabledSharedRefLogger.java
@@ -28,10 +28,10 @@
public void logProjectDelete(String project) {}
@Override
- public void logLockAcquisition(String project, String refName) {}
+ public void logLockAcquisition(String project, String refName, Scope scope) {}
@Override
- public void logLockRelease(String project, String refName) {}
+ public void logLockRelease(String project, String refName, Scope scope) {}
@Override
public <T> void logRefUpdate(String project, String refName, T currRef, T newRefValue) {}
diff --git a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLoggerTest.java b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLoggerTest.java
index 47964cf..5b9a9dd 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLoggerTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/Log4jSharedRefLoggerTest.java
@@ -120,7 +120,7 @@
@Test
public void shouldLogLockAcquisition() {
String refName = "refs/foo/bar";
- log4jSharedRefLogger.logLockAcquisition(project.get(), refName);
+ log4jSharedRefLogger.logLockAcquisition(project.get(), refName, SharedRefLogger.Scope.GLOBAL);
SharedRefLogEntry.LockAcquire gotLogEntry =
gson.fromJson(logWriter.toString(), SharedRefLogEntry.LockAcquire.class);
@@ -133,7 +133,7 @@
@Test
public void shouldLogLockRelease() {
String refName = "refs/foo/bar";
- log4jSharedRefLogger.logLockRelease(project.get(), refName);
+ log4jSharedRefLogger.logLockRelease(project.get(), refName, SharedRefLogger.Scope.GLOBAL);
SharedRefLogEntry.LockAcquire gotLogEntry =
gson.fromJson(logWriter.toString(), SharedRefLogEntry.LockAcquire.class);
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 1a47aa5..2e5f4ff 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/RefUpdateValidatorTest.java
@@ -18,12 +18,14 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError;
+import com.gerritforge.gerrit.globalrefdb.RefDbLockException;
import com.gerritforge.gerrit.globalrefdb.validation.RefUpdateValidator.OneParameterFunction;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.DefaultSharedRefEnforcement;
import com.gerritforge.gerrit.globalrefdb.validation.dfsrefdb.RefFixture;
@@ -94,7 +96,7 @@
@Test
public void validationShouldSucceedWhenSharedRefDbIsNoop() throws Exception {
SharedRefDatabaseWrapper noopSharedRefDbWrapper =
- new SharedRefDatabaseWrapper(sharedRefLogger, sharedRefDBMetrics);
+ new SharedRefDatabaseWrapper(sharedRefLogger, sharedRefDBMetrics, NoOpRefLocker.INSTANCE);
Result result =
newRefUpdateValidator(noopSharedRefDbWrapper)
@@ -179,6 +181,18 @@
}
@Test
+ public void validationShouldFailWhenLocalRefDbIsLocked() throws Exception {
+ doThrow(RefDbLockException.class)
+ .when(sharedRefDb)
+ .lockLocalRef(A_TEST_PROJECT_NAME_KEY, refName);
+
+ Result result =
+ refUpdateValidator.executeRefUpdate(refUpdate, () -> Result.NEW, this::defaultRollback);
+
+ assertThat(result).isEqualTo(Result.LOCK_FAILURE);
+ }
+
+ @Test
public void shouldRollbackWhenLocalRefDbIsUpToDateButFinalCompareAndPutIsFailing()
throws Exception {
lenient()
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 884090e..d2e52fd 100644
--- a/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java
+++ b/src/test/java/com/gerritforge/gerrit/globalrefdb/validation/SharedRefDatabaseWrapperTest.java
@@ -47,7 +47,8 @@
when(metrics.startExistsExecutionTime()).thenReturn(context);
when(metrics.startIsUpToDateExecutionTime()).thenReturn(context);
when(metrics.startRemoveExecutionTime()).thenReturn(context);
- objectUnderTest = new SharedRefDatabaseWrapper(sharedRefLogger, metrics);
+ objectUnderTest =
+ new SharedRefDatabaseWrapper(sharedRefLogger, metrics, NoOpRefLocker.INSTANCE);
}
@Test
