diff --git a/core/src/main/java/google/registry/model/EntityClasses.java b/core/src/main/java/google/registry/model/EntityClasses.java index 33ba9fc7e..782241db9 100644 --- a/core/src/main/java/google/registry/model/EntityClasses.java +++ b/core/src/main/java/google/registry/model/EntityClasses.java @@ -28,7 +28,6 @@ import google.registry.model.index.EppResourceIndex; import google.registry.model.index.EppResourceIndexBucket; import google.registry.model.index.ForeignKeyIndex; import google.registry.model.reporting.HistoryEntry; -import google.registry.model.server.Lock; import google.registry.model.server.ServerSecret; /** Sets of classes of the Objectify-registered entities in use throughout the model. */ @@ -52,7 +51,6 @@ public final class EntityClasses { HistoryEntry.class, Host.class, HostHistory.class, - Lock.class, ServerSecret.class); private EntityClasses() {} diff --git a/core/src/main/java/google/registry/model/server/Lock.java b/core/src/main/java/google/registry/model/server/Lock.java index fedd47a0d..072b62c80 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -16,7 +16,6 @@ package google.registry.model.server; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.DateTimeUtils.isAtOrAfter; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; @@ -24,15 +23,8 @@ import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; -import com.googlecode.objectify.Key; -import com.googlecode.objectify.annotation.Entity; -import com.googlecode.objectify.annotation.Id; import google.registry.model.ImmutableObject; -import google.registry.model.annotations.NotBackedUp; -import google.registry.model.annotations.NotBackedUp.Reason; import google.registry.persistence.VKey; -import google.registry.persistence.transaction.JpaTransactionManager; -import google.registry.persistence.transaction.TransactionManager; import google.registry.util.RequestStatusChecker; import google.registry.util.RequestStatusCheckerImpl; import java.io.Serializable; @@ -40,6 +32,8 @@ import java.util.Optional; import java.util.function.Supplier; import javax.annotation.Nullable; import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.Id; import javax.persistence.IdClass; import javax.persistence.PostLoad; import javax.persistence.Table; @@ -57,8 +51,6 @@ import org.joda.time.Duration; * safe calls that automatically lock and unlock, see LockHandler. */ @Entity -@NotBackedUp(reason = Reason.TRANSIENT) -@javax.persistence.Entity @Table @IdClass(Lock.LockId.class) public class Lock extends ImmutableObject implements Serializable { @@ -109,14 +101,13 @@ public class Lock extends ImmutableObject implements Serializable { /** The resource name used to create the lock. */ @Column(nullable = false) - @javax.persistence.Id + @Id String resourceName; /** The tld used to create the lock, or GLOBAL if it's cross-TLD. */ - // TODO(b/177567432): rename to "scope" post-Datastore - @Column(nullable = false, name = "scope") - @javax.persistence.Id - String tld; + @Column(nullable = false) + @Id + String scope; public DateTime getExpirationTime() { return expirationTime; @@ -124,7 +115,7 @@ public class Lock extends ImmutableObject implements Serializable { @PostLoad void postLoad() { - lockId = makeLockId(resourceName, tld); + lockId = makeLockId(resourceName, scope); } /** @@ -146,7 +137,7 @@ public class Lock extends ImmutableObject implements Serializable { instance.expirationTime = acquiredTime.plus(leaseLength); instance.acquiredTime = acquiredTime; instance.resourceName = resourceName; - instance.tld = scope; + instance.scope = scope; return instance; } @@ -157,8 +148,13 @@ public class Lock extends ImmutableObject implements Serializable { @AutoValue abstract static class AcquireResult { public abstract DateTime transactionTime(); - public abstract @Nullable Lock existingLock(); - public abstract @Nullable Lock newLock(); + + @Nullable + public abstract Lock existingLock(); + + @Nullable + public abstract Lock newLock(); + public abstract LockState lockState(); public static AcquireResult create( @@ -213,64 +209,18 @@ public class Lock extends ImmutableObject implements Serializable { Duration leaseLength, RequestStatusChecker requestStatusChecker, boolean checkThreadRunning) { - return acquireWithTransactionManager( - resourceName, tld, leaseLength, requestStatusChecker, checkThreadRunning, tm()); - } - - /** - * Try to acquire a lock in SQL. Returns absent if it can't be acquired. - * - *

This method exists so that Beam pipelines can acquire / load / release locks. - */ - public static Optional acquireSql( - String resourceName, - @Nullable String tld, - Duration leaseLength, - RequestStatusChecker requestStatusChecker, - boolean checkThreadRunning) { - return acquireWithTransactionManager( - resourceName, tld, leaseLength, requestStatusChecker, checkThreadRunning, jpaTm()); - } - - /** Release the lock. */ - public void release() { - releaseWithTransactionManager(tm()); - } - - /** - * Release the lock from SQL. - * - *

This method exists so that Beam pipelines can acquire / load / release locks. - */ - public void releaseSql() { - releaseWithTransactionManager(jpaTm()); - } - - /** Try to acquire a lock. Returns absent if it can't be acquired. */ - private static Optional acquireWithTransactionManager( - String resourceName, - @Nullable String tld, - Duration leaseLength, - RequestStatusChecker requestStatusChecker, - boolean checkThreadRunning, - TransactionManager transactionManager) { - String scope = (tld != null) ? tld : GLOBAL; - String lockId = makeLockId(resourceName, scope); + String scope = tld != null ? tld : GLOBAL; // It's important to use transactNew rather than transact, because a Lock can be used to control // access to resources like GCS that can't be transactionally rolled back. Therefore, the lock // must be definitively acquired before it is used, even when called inside another transaction. Supplier lockAcquirer = () -> { - DateTime now = transactionManager.getTransactionTime(); + DateTime now = jpaTm().getTransactionTime(); // Checking if an unexpired lock still exists - if so, the lock can't be acquired. Lock lock = - transactionManager - .loadByKeyIfPresent( - VKey.create( - Lock.class, - new LockId(resourceName, scope), - Key.create(Lock.class, lockId))) + jpaTm() + .loadByKeyIfPresent(VKey.createSql(Lock.class, new LockId(resourceName, scope))) .orElse(null); if (lock != null) { logger.atInfo().log( @@ -292,15 +242,11 @@ public class Lock extends ImmutableObject implements Serializable { create(resourceName, scope, requestStatusChecker.getLogId(), now, leaseLength); // Locks are not parented under an EntityGroupRoot (so as to avoid write // contention) and don't need to be backed up. - transactionManager.put(newLock); + jpaTm().put(newLock); return AcquireResult.create(now, lock, newLock, lockState); }; - // In ofy, backup is determined per-action, but in SQL it's determined per-transaction - AcquireResult acquireResult = - transactionManager.isOfy() - ? transactionManager.transactNew(lockAcquirer) - : ((JpaTransactionManager) transactionManager).transactWithoutBackup(lockAcquirer); + AcquireResult acquireResult = jpaTm().transactWithoutBackup(lockAcquirer); logAcquireResult(acquireResult); lockMetrics.recordAcquire(resourceName, scope, acquireResult.lockState()); @@ -308,62 +254,51 @@ public class Lock extends ImmutableObject implements Serializable { } /** Release the lock. */ - private void releaseWithTransactionManager(TransactionManager transactionManager) { + public void release() { // Just use the default clock because we aren't actually doing anything that will use the clock. Supplier lockReleaser = () -> { // To release a lock, check that no one else has already obtained it and if not - // delete it. If the lock in Datastore was different then this lock is gone already; + // delete it. If the lock in the database was different, then this lock is gone already; // this can happen if release() is called around the expiration time and the lock // expires underneath us. - VKey key = - VKey.create( - Lock.class, new LockId(resourceName, tld), Key.create(Lock.class, lockId)); - Lock loadedLock = transactionManager.loadByKeyIfPresent(key).orElse(null); - if (Lock.this.equals(loadedLock)) { + VKey key = VKey.createSql(Lock.class, new LockId(resourceName, scope)); + Lock loadedLock = jpaTm().loadByKeyIfPresent(key).orElse(null); + if (equals(loadedLock)) { // Use deleteIgnoringReadOnly() so that we don't create a commit log entry for deleting // the lock. logger.atInfo().log("Deleting lock: %s", lockId); - transactionManager.delete(key); + jpaTm().delete(key); lockMetrics.recordRelease( - resourceName, - tld, - new Duration(acquiredTime, transactionManager.getTransactionTime())); + resourceName, scope, new Duration(acquiredTime, jpaTm().getTransactionTime())); } else { logger.atSevere().log( "The lock we acquired was transferred to someone else before we" + " released it! Did action take longer than lease length?" + " Our lock: %s, current lock: %s", - Lock.this, loadedLock); + this, loadedLock); logger.atInfo().log( "Not deleting lock: %s - someone else has it: %s", lockId, loadedLock); } return null; }; - - // In ofy, backup is determined per-action, but in SQL it's determined per-transaction - if (transactionManager.isOfy()) { - transactionManager.transact(lockReleaser); - } else { - ((JpaTransactionManager) transactionManager).transactWithoutBackup(lockReleaser); - } + jpaTm().transactWithoutBackup(lockReleaser); } static class LockId extends ImmutableObject implements Serializable { String resourceName; - // TODO(b/177567432): rename to "scope" post-Datastore - @Column(name = "scope") - String tld; + String scope; // Required for Hibernate + @SuppressWarnings("unused") private LockId() {} - LockId(String resourceName, String tld) { + LockId(String resourceName, String scope) { this.resourceName = checkArgumentNotNull(resourceName, "The resource name cannot be null"); - this.tld = checkArgumentNotNull(tld, "Scope of a lock cannot be null"); + this.scope = checkArgumentNotNull(scope, "Scope of a lock cannot be null"); } } } diff --git a/core/src/main/java/google/registry/request/lock/LockHandler.java b/core/src/main/java/google/registry/request/lock/LockHandler.java index 2c15d5e04..31ed7fec0 100644 --- a/core/src/main/java/google/registry/request/lock/LockHandler.java +++ b/core/src/main/java/google/registry/request/lock/LockHandler.java @@ -38,26 +38,5 @@ public interface LockHandler extends Serializable { * @return true if all locks were acquired and the callable was run; false otherwise. */ boolean executeWithLocks( - final Callable callable, - @Nullable String tld, - Duration leaseLength, - String... lockNames); - - /** - * Acquire one or more locks using only Cloud SQL and execute a Void {@link Callable}. - * - *

Runs on a thread that will be killed if it doesn't complete before the lease expires. - * - *

Note that locks are specific either to a given tld or to the entire system (in which case - * tld should be passed as null). - * - *

This method exists so that Beam pipelines can acquire / load / release locks. - * - * @return true if all locks were acquired and the callable was run; false otherwise. - */ - boolean executeWithSqlLocks( - final Callable callable, - @Nullable String tld, - Duration leaseLength, - String... lockNames); + Callable callable, @Nullable String tld, Duration leaseLength, String... lockNames); } diff --git a/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java b/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java index 12d0e2396..66be0b775 100644 --- a/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java +++ b/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java @@ -76,27 +76,6 @@ public class LockHandlerImpl implements LockHandler { return executeWithLockAcquirer(callable, tld, leaseLength, this::acquire, lockNames); } - /** - * Acquire one or more locks using only Cloud SQL and execute a Void {@link Callable}. - * - *

Thread will be killed if it doesn't complete before the lease expires. - * - *

Note that locks are specific either to a given tld or to the entire system (in which case - * tld should be passed as null). - * - *

This method exists so that Beam pipelines can acquire / load / release locks. - * - * @return whether all locks were acquired and the callable was run. - */ - @Override - public boolean executeWithSqlLocks( - final Callable callable, - @Nullable String tld, - Duration leaseLength, - String... lockNames) { - return executeWithLockAcquirer(callable, tld, leaseLength, this::acquireSql, lockNames); - } - private boolean executeWithLockAcquirer( final Callable callable, @Nullable String tld, @@ -138,11 +117,6 @@ public class LockHandlerImpl implements LockHandler { return Lock.acquire(lockName, tld, leaseLength, requestStatusChecker, true); } - @VisibleForTesting - Optional acquireSql(String lockName, @Nullable String tld, Duration leaseLength) { - return Lock.acquireSql(lockName, tld, leaseLength, requestStatusChecker, true); - } - private interface LockAcquirer { Optional acquireLock(String lockName, @Nullable String tld, Duration leaseLength); } diff --git a/core/src/test/java/google/registry/model/common/ClassPathManagerTest.java b/core/src/test/java/google/registry/model/common/ClassPathManagerTest.java index 179e56ea4..373f9e615 100644 --- a/core/src/test/java/google/registry/model/common/ClassPathManagerTest.java +++ b/core/src/test/java/google/registry/model/common/ClassPathManagerTest.java @@ -27,7 +27,6 @@ import google.registry.model.index.ForeignKeyIndex.ForeignKeyContactIndex; import google.registry.model.index.ForeignKeyIndex.ForeignKeyDomainIndex; import google.registry.model.index.ForeignKeyIndex.ForeignKeyHostIndex; import google.registry.model.reporting.HistoryEntry; -import google.registry.model.server.Lock; import google.registry.model.server.ServerSecret; import google.registry.testing.TestObject; import org.junit.jupiter.api.Test; @@ -53,7 +52,6 @@ public class ClassPathManagerTest { assertThat(ClassPathManager.getClass("EppResourceIndexBucket")) .isEqualTo(EppResourceIndexBucket.class); assertThat(ClassPathManager.getClass("EntityGroupRoot")).isEqualTo(EntityGroupRoot.class); - assertThat(ClassPathManager.getClass("Lock")).isEqualTo(Lock.class); assertThat(ClassPathManager.getClass("Domain")).isEqualTo(Domain.class); assertThat(ClassPathManager.getClass("HistoryEntry")).isEqualTo(HistoryEntry.class); assertThat(ClassPathManager.getClass("ForeignKeyHostIndex")) @@ -103,7 +101,6 @@ public class ClassPathManagerTest { assertThat(ClassPathManager.getClassName(EppResourceIndexBucket.class)) .isEqualTo("EppResourceIndexBucket"); assertThat(ClassPathManager.getClassName(EntityGroupRoot.class)).isEqualTo("EntityGroupRoot"); - assertThat(ClassPathManager.getClassName(Lock.class)).isEqualTo("Lock"); assertThat(ClassPathManager.getClassName(Domain.class)).isEqualTo("Domain"); assertThat(ClassPathManager.getClassName(HistoryEntry.class)).isEqualTo("HistoryEntry"); assertThat(ClassPathManager.getClassName(ForeignKeyHostIndex.class)) diff --git a/core/src/test/java/google/registry/model/server/LockTest.java b/core/src/test/java/google/registry/model/server/LockTest.java index 79864f3dc..567e3e380 100644 --- a/core/src/test/java/google/registry/model/server/LockTest.java +++ b/core/src/test/java/google/registry/model/server/LockTest.java @@ -49,7 +49,8 @@ public class LockTest extends EntityTestCase { super(JpaEntityCoverageCheck.ENABLED); } - private Optional acquire(String tld, Duration leaseLength, LockState expectedLockState) { + private static Optional acquire( + String tld, Duration leaseLength, LockState expectedLockState) { Lock.lockMetrics = mock(LockMetrics.class); Optional lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength, requestStatusChecker, true); verify(Lock.lockMetrics).recordAcquire(RESOURCE_NAME, tld, expectedLockState); @@ -58,7 +59,7 @@ public class LockTest extends EntityTestCase { return lock; } - private void release(Lock lock, String expectedTld, long expectedMillis) { + private static void release(Lock lock, String expectedTld, long expectedMillis) { Lock.lockMetrics = mock(LockMetrics.class); lock.release(); verify(Lock.lockMetrics) diff --git a/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java b/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java index 982dcf3f4..21601f344 100644 --- a/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java +++ b/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java @@ -44,7 +44,7 @@ final class LockHandlerImplTest { final AppEngineExtension appEngine = AppEngineExtension.builder().withCloudSql().build(); private static class CountingCallable implements Callable { - int numCalled = 0; + int numCalled; @Override public Void call() { @@ -69,16 +69,11 @@ final class LockHandlerImplTest { } } - private boolean executeWithLocks(Callable callable, final @Nullable Lock acquiredLock) { + private boolean executeWithLocks(Callable callable, @Nullable final Lock acquiredLock) { return createTestLockHandler(acquiredLock) .executeWithLocks(callable, "tld", ONE_DAY, "resourceName"); } - private boolean executeWithSqlLocks(Callable callable, final @Nullable Lock acquiredLock) { - return createTestLockHandler(acquiredLock) - .executeWithSqlLocks(callable, "tld", ONE_DAY, "resourceName"); - } - @Test void testLockSucceeds() { Lock lock = mock(Lock.class); @@ -88,15 +83,6 @@ final class LockHandlerImplTest { verify(lock, times(1)).release(); } - @Test - void testSqlLockSucceeds() { - Lock lock = mock(Lock.class); - CountingCallable countingCallable = new CountingCallable(); - assertThat(executeWithSqlLocks(countingCallable, lock)).isTrue(); - assertThat(countingCallable.numCalled).isEqualTo(1); - verify(lock, times(1)).release(); - } - @Test void testLockSucceeds_uncheckedException() { Lock lock = mock(Lock.class); @@ -157,11 +143,6 @@ final class LockHandlerImplTest { assertThat(leaseLength).isEqualTo(ONE_DAY); return Optional.ofNullable(acquiredLock); } - - @Override - Optional acquireSql(String resourceName, String tld, Duration leaseLength) { - return acquire(resourceName, tld, leaseLength); - } }; } } diff --git a/core/src/test/java/google/registry/testing/FakeLockHandler.java b/core/src/test/java/google/registry/testing/FakeLockHandler.java index eb024921c..c2382befb 100644 --- a/core/src/test/java/google/registry/testing/FakeLockHandler.java +++ b/core/src/test/java/google/registry/testing/FakeLockHandler.java @@ -42,12 +42,6 @@ public class FakeLockHandler implements LockHandler { return execute(callable); } - @Override - public boolean executeWithSqlLocks( - Callable callable, @Nullable String tld, Duration leaseLength, String... lockNames) { - return execute(callable); - } - private boolean execute(Callable callable) { if (!lockSucceeds) { return false; diff --git a/core/src/test/resources/google/registry/model/schema.txt b/core/src/test/resources/google/registry/model/schema.txt index e91f1a39d..7ca42ceff 100644 --- a/core/src/test/resources/google/registry/model/schema.txt +++ b/core/src/test/resources/google/registry/model/schema.txt @@ -389,14 +389,6 @@ enum google.registry.model.reporting.HistoryEntry$Type { RDE_IMPORT; SYNTHETIC; } -class google.registry.model.server.Lock { - @Id java.lang.String lockId; - java.lang.String requestLogId; - java.lang.String resourceName; - java.lang.String tld; - org.joda.time.DateTime acquiredTime; - org.joda.time.DateTime expirationTime; -} class google.registry.model.server.ServerSecret { @Id long id; @Parent com.googlecode.objectify.Key parent;