Remove ofy from Lock (#1771)

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/google/nomulus/1771)
<!-- Reviewable:end -->

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

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)
-  @Column(nullable = false, name = "scope")
+  @Id
-  @javax.persistence.Id
+  String scope;
-  String tld;
 
   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(
+    String scope = tld != null ? tld : GLOBAL;
-        resourceName, tld, leaseLength, requestStatusChecker, checkThreadRunning, tm());
-  }
-
-  /**
-   * Try to acquire a lock in SQL. Returns absent if it can't be acquired.
-   *
-   * <p>This method exists so that Beam pipelines can acquire / load / release locks.
-   */
-  public static Optional<Lock> 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.
-   *
-   * <p>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<Lock> 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);
     // 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<AcquireResult> 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
+              jpaTm()
-                  .loadByKeyIfPresent(
+                  .loadByKeyIfPresent(VKey.createSql(Lock.class, new LockId(resourceName, scope)))
-                      VKey.create(
-                          Lock.class,
-                          new LockId(resourceName, scope),
-                          Key.create(Lock.class, lockId)))
                   .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 = jpaTm().transactWithoutBackup(lockAcquirer);
-    AcquireResult acquireResult =
-        transactionManager.isOfy()
-            ? transactionManager.transactNew(lockAcquirer)
-            : ((JpaTransactionManager) transactionManager).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<Void> 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<Lock> key =
+          VKey<Lock> key = VKey.createSql(Lock.class, new LockId(resourceName, scope));
-              VKey.create(
+          Lock loadedLock = jpaTm().loadByKeyIfPresent(key).orElse(null);
-                  Lock.class, new LockId(resourceName, tld), Key.create(Lock.class, lockId));
+          if (equals(loadedLock)) {
-          Lock loadedLock = transactionManager.loadByKeyIfPresent(key).orElse(null);
-          if (Lock.this.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,
+                resourceName, scope, new Duration(acquiredTime, jpaTm().getTransactionTime()));
-                tld,
-                new Duration(acquiredTime, transactionManager.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;
         };
-
+    jpaTm().transactWithoutBackup(lockReleaser);
-    // 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);
-    }
   }
 
   static class LockId extends ImmutableObject implements Serializable {
 
     String resourceName;
 
-    // TODO(b/177567432): rename to "scope" post-Datastore
+    String scope;
-    @Column(name = "scope")
-    String tld;
 
     // 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");
     }
   }
 }

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<Void> callable,
+      Callable<Void> callable, @Nullable String tld, Duration leaseLength, String... lockNames);
-      @Nullable String tld,
-      Duration leaseLength,
-      String... lockNames);
-
-  /**
-   * Acquire one or more locks using only Cloud SQL and execute a Void {@link Callable}.
-   *
-   * <p>Runs on a thread that will be killed if it doesn't complete before the lease expires.
-   *
-   * <p>Note that locks are specific either to a given tld or to the entire system (in which case
-   * tld should be passed as null).
-   *
-   * <p>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<Void> callable,
-      @Nullable String tld,
-      Duration leaseLength,
-      String... lockNames);
 }

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}.
-   *
-   * <p>Thread will be killed if it doesn't complete before the lease expires.
-   *
-   * <p>Note that locks are specific either to a given tld or to the entire system (in which case
-   * tld should be passed as null).
-   *
-   * <p>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<Void> callable,
-      @Nullable String tld,
-      Duration leaseLength,
-      String... lockNames) {
-    return executeWithLockAcquirer(callable, tld, leaseLength, this::acquireSql, lockNames);
-  }
-
   private boolean executeWithLockAcquirer(
       final Callable<Void> callable,
       @Nullable String tld,
@@ -138,11 +117,6 @@ public class LockHandlerImpl implements LockHandler {
     return Lock.acquire(lockName, tld, leaseLength, requestStatusChecker, true);
   }
 
-  @VisibleForTesting
-  Optional<Lock> acquireSql(String lockName, @Nullable String tld, Duration leaseLength) {
-    return Lock.acquireSql(lockName, tld, leaseLength, requestStatusChecker, true);
-  }
-
   private interface LockAcquirer {
     Optional<Lock> acquireLock(String lockName, @Nullable String tld, Duration leaseLength);
   }

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))

core/src/test/java/google/registry/model/server/LockTest.java

@@ -49,7 +49,8 @@ public class LockTest extends EntityTestCase {
     super(JpaEntityCoverageCheck.ENABLED);
   }
 
-  private Optional<Lock> acquire(String tld, Duration leaseLength, LockState expectedLockState) {
+  private static Optional<Lock> acquire(
+      String tld, Duration leaseLength, LockState expectedLockState) {
     Lock.lockMetrics = mock(LockMetrics.class);
     Optional<Lock> 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)

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<Void> {
-    int numCalled = 0;
+    int numCalled;
 
     @Override
     public Void call() {
@@ -69,16 +69,11 @@ final class LockHandlerImplTest {
     }
   }
 
-  private boolean executeWithLocks(Callable<Void> callable, final @Nullable Lock acquiredLock) {
+  private boolean executeWithLocks(Callable<Void> callable, @Nullable final Lock acquiredLock) {
     return createTestLockHandler(acquiredLock)
         .executeWithLocks(callable, "tld", ONE_DAY, "resourceName");
   }
 
-  private boolean executeWithSqlLocks(Callable<Void> 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<Lock> acquireSql(String resourceName, String tld, Duration leaseLength) {
-        return acquire(resourceName, tld, leaseLength);
-      }
     };
   }
 }

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<Void> callable, @Nullable String tld, Duration leaseLength, String... lockNames) {
-    return execute(callable);
-  }
-
   private boolean execute(Callable<Void> callable) {
     if (!lockSucceeds) {
       return false;

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<google.registry.model.common.EntityGroupRoot> parent;