Add Lock dual write (#496)

* Add Lock dual write

* wrap calls in DB transaction

core/src/main/java/google/registry/model/server/Lock.java

@@ -16,6 +16,7 @@ package google.registry.model.server;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.DateTimeUtils.isAtOrAfter;
 
@@ -28,6 +29,7 @@ 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.schema.server.LockDao;
 import google.registry.util.RequestStatusChecker;
 import google.registry.util.RequestStatusCheckerImpl;
 import java.io.Serializable;
@@ -177,8 +179,7 @@ public class Lock extends ImmutableObject implements Serializable {
     // 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.
     AcquireResult acquireResult =
-        tm()
+        tm().transactNew(
-            .transactNew(
                 () -> {
                   DateTime now = tm().getTransactionTime();
 
@@ -207,6 +208,31 @@ public class Lock extends ImmutableObject implements Serializable {
                   // contention) and
                   // don't need to be backed up.
                   ofy().saveWithoutBackup().entity(newLock);
+
+                  // create and save the lock to Cloud SQL
+                  try {
+                    jpaTm()
+                        .transact(
+                            () -> {
+                              google.registry.schema.server.Lock cloudSqlLock =
+                                  google.registry.schema.server.Lock.create(
+                                      resourceName,
+                                      Optional.ofNullable(tld).orElse("GLOBAL"),
+                                      requestStatusChecker.getLogId(),
+                                      now,
+                                      leaseLength);
+                              // cloudSqlLock should not already exist in Cloud SQL, but call delete
+                              // just in case
+                              // TODO: Remove this delete once dual read is added
+                              LockDao.delete(
+                                  resourceName, Optional.ofNullable(tld).orElse("GLOBAL"));
+                              LockDao.saveNew(cloudSqlLock);
+                            });
+                  } catch (Exception e) {
+                    logger.atSevere().withCause(e).log(
+                        "Error saving lock to Cloud SQL: %s", newLock);
+                  }
+
                   return AcquireResult.create(now, lock, newLock, lockState);
                 });
 
@@ -218,8 +244,7 @@ public class Lock extends ImmutableObject implements Serializable {
   /** Release the lock. */
   public void release() {
     // Just use the default clock because we aren't actually doing anything that will use the clock.
-    tm()
+    tm().transact(
-        .transact(
             () -> {
               // 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;
@@ -231,6 +256,19 @@ public class Lock extends ImmutableObject implements Serializable {
                 // lock.
                 logger.atInfo().log("Deleting lock: %s", lockId);
                 ofy().deleteWithoutBackup().entity(Lock.this);
+
+                // Remove the lock from Cloud SQL
+                try {
+                  jpaTm()
+                      .transact(
+                          () ->
+                              LockDao.delete(
+                                  resourceName, Optional.ofNullable(tld).orElse("GLOBAL")));
+                } catch (Exception e) {
+                  logger.atSevere().withCause(e).log(
+                      "Error deleting lock from Cloud SQL: %s", loadedLock);
+                }
+
                 lockMetrics.recordRelease(
                     resourceName, tld, new Duration(acquiredTime, tm().getTransactionTime()));
               } else {

core/src/main/java/google/registry/schema/server/LockDao.java

@@ -51,26 +51,29 @@ public class LockDao {
    * else empty.
    */
   public static Optional<Lock> load(String resourceName) {
-    checkArgumentNotNull(resourceName, "The resource name of the lock to load cannot be null");
+    return load(resourceName, GLOBAL);
-    return Optional.ofNullable(
-        jpaTm()
-            .transact(
-                () ->
-                    jpaTm().getEntityManager().find(Lock.class, new LockId(resourceName, GLOBAL))));
   }
 
   /**
-   * Deletes the given {@link Lock} object from Cloud SQL. This method is idempotent and will simply
+   * Deletes the {@link Lock} object with the given resourceName and tld from Cloud SQL. This method
-   * return if the lock has already been deleted.
+   * is idempotent and will simply return if the lock has already been deleted.
    */
-  public static void delete(Lock lock) {
+  public static void delete(String resourceName, String tld) {
     jpaTm()
         .transact(
             () -> {
-              Optional<Lock> loadedLock = load(lock.resourceName, lock.tld);
+              Optional<Lock> loadedLock = load(resourceName, tld);
               if (loadedLock.isPresent()) {
                 jpaTm().getEntityManager().remove(loadedLock.get());
               }
             });
   }
+
+  /**
+   * Deletes the global {@link Lock} object with the given resourceName from Cloud SQL. This method
+   * is idempotent and will simply return if the lock has already been deleted.
+   */
+  public static void delete(String resourceName) {
+    delete(resourceName, GLOBAL);
+  }
 }

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

@@ -28,6 +28,9 @@ import static org.mockito.Mockito.when;
 
 import google.registry.model.ofy.Ofy;
 import google.registry.model.server.Lock.LockState;
+import google.registry.persistence.transaction.JpaTestRules;
+import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageRule;
+import google.registry.schema.server.LockDao;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectRule;
@@ -56,21 +59,28 @@ public class LockTest {
   @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build();
   @Rule public final InjectRule inject = new InjectRule();
 
+  @Rule
+  public final JpaIntegrationWithCoverageRule jpaRule =
+      new JpaTestRules.Builder().withClock(clock).buildIntegrationWithCoverageRule();
+
   private 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);
     verifyNoMoreInteractions(Lock.lockMetrics);
+    assertThat(LockDao.load(RESOURCE_NAME, tld)).isPresent();
     Lock.lockMetrics = null;
     return lock;
   }
 
   private void release(Lock lock, String expectedTld, long expectedMillis) {
+    assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isPresent();
     Lock.lockMetrics = mock(LockMetrics.class);
     lock.release();
     verify(Lock.lockMetrics)
         .recordRelease(RESOURCE_NAME, expectedTld, Duration.millis(expectedMillis));
     verifyNoMoreInteractions(Lock.lockMetrics);
+    assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isEmpty();
     Lock.lockMetrics = null;
   }
 

core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java

@@ -18,6 +18,7 @@ import com.google.common.truth.Expect;
 import google.registry.model.common.CursorTest;
 import google.registry.model.domain.DomainBaseSqlTest;
 import google.registry.model.registry.RegistryLockDaoTest;
+import google.registry.model.server.LockTest;
 import google.registry.persistence.transaction.JpaEntityCoverage;
 import google.registry.schema.cursor.CursorDaoTest;
 import google.registry.schema.registrar.RegistrarDaoTest;
@@ -64,10 +65,11 @@ import org.junit.runners.Suite.SuiteClasses;
   CreateReservedListCommandTest.class,
   CursorDaoTest.class,
   CursorTest.class,
+  DomainBaseSqlTest.class,
   DomainLockUtilsTest.class,
   LockDaoTest.class,
   LockDomainCommandTest.class,
-  DomainBaseSqlTest.class,
+  LockTest.class,
   PremiumListDaoTest.class,
   RegistrarDaoTest.class,
   RegistryLockDaoTest.class,

core/src/test/java/google/registry/schema/server/LockDaoTest.java

@@ -103,7 +103,7 @@ public class LockDaoTest {
     LockDao.saveNew(lock);
     Optional<Lock> returnedLock = LockDao.load("testResource", "tld");
     assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    LockDao.delete(lock);
+    LockDao.delete("testResource", "tld");
     returnedLock = LockDao.load("testResource", "tld");
     assertThat(returnedLock.isPresent()).isFalse();
   }
@@ -115,15 +115,13 @@ public class LockDaoTest {
     LockDao.saveNew(lock);
     Optional<Lock> returnedLock = LockDao.load("testResource");
     assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime);
-    LockDao.delete(lock);
+    LockDao.delete("testResource");
     returnedLock = LockDao.load("testResource");
     assertThat(returnedLock.isPresent()).isFalse();
   }
 
   @Test
   public void delete_succeedsLockDoesntExist() {
-    Lock lock =
+    LockDao.delete("testResource");
-        Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2));
-    LockDao.delete(lock);
   }
 }