From 92dcacf78c6fb0bea65ccc0aa2926649ef5b41e5 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Mon, 12 Apr 2021 12:11:20 -0400 Subject: [PATCH] Add a beforeSqlSave callback to ReplaySpecializer (#1062) * Add a beforeSqlSave callback to ReplaySpecializer When in the Datastore-primary and SQL-secondary stage, we will want to save the EppResource-at-this-point-in-time field in the *History objects so that later on we can examine the *History objects to see what the resource looked like at that point in time. Without this PR, the full object at that point in time would be lost during the asynchronous replay since Datastore doesn't know about it. In addition, we modify the HistoryEntry weight / priority so that additions to it come after the additions to the resource off of which it is based. As a result, we need to DEFER some foreign keys so that we can write the billing / poll message objects before the history object that they're referencing. --- .../backup/ReplayCommitLogsToSqlAction.java | 8 +- .../model/contact/ContactHistory.java | 8 + .../registry/model/domain/DomainHistory.java | 7 + .../registry/model/host/HostHistory.java | 8 + .../model/ofy/EntityWritePriorities.java | 7 +- .../schema/replay/ReplaySpecializer.java | 21 ++- .../ReplayCommitLogsToSqlActionTest.java | 16 ++ .../model/ofy/EntityWritePrioritiesTest.java | 2 +- .../google/registry/testing/TestObject.java | 5 + .../sql/er_diagram/brief_er_diagram.html | 6 +- .../sql/er_diagram/full_er_diagram.html | 148 +++++++++--------- db/src/main/resources/sql/flyway.txt | 1 + .../resources/sql/flyway/V91__defer_fkeys.sql | 48 ++++++ .../resources/sql/schema/nomulus.golden.sql | 10 +- 14 files changed, 202 insertions(+), 93 deletions(-) create mode 100644 db/src/main/resources/sql/flyway/V91__defer_fkeys.sql diff --git a/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java b/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java index 978230c3f..1803117fc 100644 --- a/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java +++ b/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java @@ -154,7 +154,13 @@ public class ReplayCommitLogsToSqlAction implements Runnable { Object ofyPojo = ofy().toPojo(entity); if (ofyPojo instanceof DatastoreEntity) { DatastoreEntity datastoreEntity = (DatastoreEntity) ofyPojo; - datastoreEntity.toSqlEntity().ifPresent(jpaTm()::put); + datastoreEntity + .toSqlEntity() + .ifPresent( + sqlEntity -> { + ReplaySpecializer.beforeSqlSave(sqlEntity); + jpaTm().put(sqlEntity); + }); } else { // this should never happen, but we shouldn't fail on it logger.atSevere().log( diff --git a/core/src/main/java/google/registry/model/contact/ContactHistory.java b/core/src/main/java/google/registry/model/contact/ContactHistory.java index d3cb17a48..feb695227 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java @@ -14,6 +14,8 @@ package google.registry.model.contact; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; + import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.EntitySubclass; import google.registry.model.ImmutableObject; @@ -114,6 +116,12 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { return Optional.of(asHistoryEntry()); } + // Used to fill out the contactBase field during asynchronous replay + public static void beforeSqlSave(ContactHistory contactHistory) { + contactHistory.contactBase = + jpaTm().loadByKey(VKey.createSql(ContactResource.class, contactHistory.getContactRepoId())); + } + /** Class to represent the composite primary key of {@link ContactHistory} entity. */ public static class ContactHistoryId extends ImmutableObject implements Serializable { diff --git a/core/src/main/java/google/registry/model/domain/DomainHistory.java b/core/src/main/java/google/registry/model/domain/DomainHistory.java index 98f6e3860..3051f20e5 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -15,6 +15,7 @@ package google.registry.model.domain; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import com.google.common.collect.ImmutableSet; @@ -263,6 +264,12 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { return Optional.of(asHistoryEntry()); } + // Used to fill out the domainContent field during asynchronous replay + public static void beforeSqlSave(DomainHistory domainHistory) { + domainHistory.domainContent = + jpaTm().loadByKey(VKey.createSql(DomainBase.class, domainHistory.getDomainRepoId())); + } + /** Class to represent the composite primary key of {@link DomainHistory} entity. */ public static class DomainHistoryId extends ImmutableObject implements Serializable { diff --git a/core/src/main/java/google/registry/model/host/HostHistory.java b/core/src/main/java/google/registry/model/host/HostHistory.java index aea9e516f..2f91349f9 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/core/src/main/java/google/registry/model/host/HostHistory.java @@ -14,6 +14,8 @@ package google.registry.model.host; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; + import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.EntitySubclass; import google.registry.model.ImmutableObject; @@ -115,6 +117,12 @@ public class HostHistory extends HistoryEntry implements SqlEntity { return Optional.of(asHistoryEntry()); } + // Used to fill out the hostBase field during asynchronous replay + public static void beforeSqlSave(HostHistory hostHistory) { + hostHistory.hostBase = + jpaTm().loadByKey(VKey.createSql(HostResource.class, hostHistory.getHostRepoId())); + } + /** Class to represent the composite primary key of {@link HostHistory} entity. */ public static class HostHistoryId extends ImmutableObject implements Serializable { diff --git a/core/src/main/java/google/registry/model/ofy/EntityWritePriorities.java b/core/src/main/java/google/registry/model/ofy/EntityWritePriorities.java index cc030d9ab..a2afbf61a 100644 --- a/core/src/main/java/google/registry/model/ofy/EntityWritePriorities.java +++ b/core/src/main/java/google/registry/model/ofy/EntityWritePriorities.java @@ -41,10 +41,11 @@ public class EntityWritePriorities { */ static final ImmutableMap CLASS_PRIORITIES = ImmutableMap.of( - "ContactResource", -15, - "HistoryEntry", -10, "AllocationToken", -9, - "DomainBase", 10); + "ContactResource", 8, + "HostResource", 9, + "DomainBase", 10, + "HistoryEntry", 20); // The beginning of the range of priority numbers reserved for delete. This must be greater than // any of the values in CLASS_PRIORITIES by enough overhead to accommodate any negative values in diff --git a/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java b/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java index 7f1ed474c..2220376b4 100644 --- a/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java +++ b/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java @@ -26,21 +26,30 @@ import java.lang.reflect.Method; * to invoke special class methods if they are present. */ public class ReplaySpecializer { + public static void beforeSqlDelete(VKey key) { + invokeMethod(key.getKind(), "beforeSqlDelete", key); + } + + public static void beforeSqlSave(SqlEntity sqlEntity) { + invokeMethod(sqlEntity.getClass(), "beforeSqlSave", sqlEntity); + } + + private static void invokeMethod(Class clazz, String methodName, Object argument) { try { - Method method = key.getKind().getMethod("beforeSqlDelete", VKey.class); - method.invoke(null, new Object[] {key}); + Method method = clazz.getMethod(methodName, argument.getClass()); + method.invoke(null, argument); } catch (NoSuchMethodException e) { // Ignore, this just means that the class doesn't need this hook. } catch (IllegalAccessException e) { throw new RuntimeException( - "beforeSqlDelete() method is defined for class " - + key.getKind().getName() - + " but is not public.", + String.format( + "%s() method is defined for class %s but is not public.", + methodName, clazz.getName()), e); } catch (InvocationTargetException e) { throw new RuntimeException( - "beforeSqlDelete() method for class " + key.getKind().getName() + " threw an exception.", + String.format("%s() method for class %s threw an exception", methodName, clazz.getName()), e); } } diff --git a/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java b/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java index e0b2d9feb..c0712b7e7 100644 --- a/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java +++ b/core/src/test/java/google/registry/backup/ReplayCommitLogsToSqlActionTest.java @@ -125,6 +125,7 @@ public class ReplayCommitLogsToSqlActionTest { action.diffLister.gcsBucket = GCS_BUCKET; action.diffLister.executor = newDirectExecutorService(); RegistryConfig.overrideCloudSqlReplayCommitLogs(true); + TestObject.beforeSqlSaveCallCount = 0; TestObject.beforeSqlDeleteCallCount = 0; } @@ -442,6 +443,21 @@ public class ReplayCommitLogsToSqlActionTest { .isEqualTo("Can't acquire SQL commit log replay lock, aborting."); } + @Test + void testSuccess_beforeSqlSaveCallback() throws Exception { + DateTime now = fakeClock.nowUtc(); + Key bucketKey = getBucketKey(1); + Key manifestKey = CommitLogManifest.createKey(bucketKey, now); + jpaTm().transact(() -> SqlReplayCheckpoint.set(now.minusMinutes(1).minusMillis(1))); + saveDiffFile( + gcsService, + createCheckpoint(now.minusMinutes(1)), + CommitLogManifest.create(bucketKey, now, null), + CommitLogMutation.create(manifestKey, TestObject.create("a"))); + runAndAssertSuccess(now.minusMinutes(1)); + assertThat(TestObject.beforeSqlSaveCallCount).isEqualTo(1); + } + @Test void testSuccess_deleteSqlCallback() throws Exception { DateTime now = fakeClock.nowUtc(); diff --git a/core/src/test/java/google/registry/model/ofy/EntityWritePrioritiesTest.java b/core/src/test/java/google/registry/model/ofy/EntityWritePrioritiesTest.java index a128299df..662b98a6f 100644 --- a/core/src/test/java/google/registry/model/ofy/EntityWritePrioritiesTest.java +++ b/core/src/test/java/google/registry/model/ofy/EntityWritePrioritiesTest.java @@ -40,7 +40,7 @@ class EntityWritePrioritiesTest { Key.create(HistoryEntry.class, 200), "fake history entry", Key.create(Registrar.class, 300), "fake registrar"); ImmutableMap expectedValues = - ImmutableMap.of(100L, EntityWritePriorities.DELETE_RANGE + 10, 200L, -10, 300L, 0); + ImmutableMap.of(100L, EntityWritePriorities.DELETE_RANGE - 20, 200L, 20, 300L, 0); for (ImmutableMap.Entry, Object> entry : actions.entrySet()) { assertThat( diff --git a/core/src/test/java/google/registry/testing/TestObject.java b/core/src/test/java/google/registry/testing/TestObject.java index 61ba36e08..f6695b7cd 100644 --- a/core/src/test/java/google/registry/testing/TestObject.java +++ b/core/src/test/java/google/registry/testing/TestObject.java @@ -34,6 +34,7 @@ import javax.persistence.Transient; @EntityForTesting public class TestObject extends ImmutableObject implements DatastoreAndSqlEntity { + public static int beforeSqlSaveCallCount; public static int beforeSqlDeleteCallCount; @Parent @Transient Key parent; @@ -74,6 +75,10 @@ public class TestObject extends ImmutableObject implements DatastoreAndSqlEntity beforeSqlDeleteCallCount++; } + public static void beforeSqlSave(TestObject testObject) { + beforeSqlSaveCallCount++; + } + /** A test @VirtualEntity model object, which should not be persisted. */ @Entity @VirtualEntity diff --git a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html index 213897f78..d2840093c 100644 --- a/db/src/main/resources/sql/er_diagram/brief_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/brief_er_diagram.html @@ -261,11 +261,11 @@ td.section { generated on - 2021-03-24 01:27:00.824998 + 2021-04-08 17:21:58.993542 last flyway file - V90__update_timestamp.sql + V91__defer_fkeys.sql @@ -284,7 +284,7 @@ td.section { generated on - 2021-03-24 01:27:00.824998 + 2021-04-08 17:21:58.993542 diff --git a/db/src/main/resources/sql/er_diagram/full_er_diagram.html b/db/src/main/resources/sql/er_diagram/full_er_diagram.html index 30e3e5d29..0bc6414ce 100644 --- a/db/src/main/resources/sql/er_diagram/full_er_diagram.html +++ b/db/src/main/resources/sql/er_diagram/full_er_diagram.html @@ -261,32 +261,32 @@ td.section { generated on - 2021-03-24 01:26:58.684653 + 2021-04-08 17:21:56.891855 last flyway file - V90__update_timestamp.sql + V91__defer_fkeys.sql

 

 

- + SchemaCrawler_Diagram - - + + generated by - + SchemaCrawler 16.10.1 - + generated on - - 2021-03-24 01:26:58.684653 + + 2021-04-08 17:21:56.891855 - + allocationtoken_a08ccbef @@ -5985,142 +5985,142 @@ td.section { registrylock_ac88663e - - + + public.RegistryLock - - + + [table] - + revision_id - + - + bigserial not null - + - + auto-incremented - - lock_completion_timestamp + + lock_completion_time - + - + timestamptz - - lock_request_timestamp + + lock_request_time - + - + timestamptz not null - + domain_name - + - + text not null - + is_superuser - + - + bool not null - + registrar_id - + - + text not null - + registrar_poc_id - + - + text - + repo_id - + - + text not null - + verification_code - + - + text not null - - unlock_request_timestamp + + unlock_request_time - + - + timestamptz - - unlock_completion_timestamp + + unlock_completion_time - + - + timestamptz - + last_update_time - + - + timestamptz not null - + relock_revision_id - + - + int8 - + relock_duration - + - + interval - + registrylock_ac88663e:w->registrylock_ac88663e:e - - - - - - - + + + + + + + fk2lhcwpxlnqijr96irylrh1707 @@ -12706,12 +12706,12 @@ td.section {
- lock_completion_timestamp + lock_completion_time timestamptz
- lock_request_timestamp + lock_request_time timestamptz not null
@@ -12746,12 +12746,12 @@ td.section {
- unlock_request_timestamp + unlock_request_time timestamptz
- unlock_completion_timestamp + unlock_completion_time timestamptz
diff --git a/db/src/main/resources/sql/flyway.txt b/db/src/main/resources/sql/flyway.txt index 289a0cfe3..205230686 100644 --- a/db/src/main/resources/sql/flyway.txt +++ b/db/src/main/resources/sql/flyway.txt @@ -88,3 +88,4 @@ V87__fix_super_domain_fk.sql V88__transfer_billing_cancellation_history_id.sql V89__host_history_host_deferred.sql V90__update_timestamp.sql +V91__defer_fkeys.sql diff --git a/db/src/main/resources/sql/flyway/V91__defer_fkeys.sql b/db/src/main/resources/sql/flyway/V91__defer_fkeys.sql new file mode 100644 index 000000000..2baebb707 --- /dev/null +++ b/db/src/main/resources/sql/flyway/V91__defer_fkeys.sql @@ -0,0 +1,48 @@ +-- Copyright 2021 The Nomulus Authors. All Rights Reserved. +-- +-- 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. + +-- We require that *History table writes come after their corresponding +-- EppResource writes when replaying transactions from Datastore. +-- The alterations here serve to break cycles necessary to write the +-- resource first. + +ALTER TABLE "BillingEvent" DROP CONSTRAINT fk_billing_event_domain_history; +ALTER TABLE "BillingEvent" ADD CONSTRAINT fk_billing_event_domain_history + FOREIGN KEY (domain_repo_id, domain_history_revision_id) + REFERENCES "DomainHistory"(domain_repo_id, history_revision_id) + DEFERRABLE INITIALLY DEFERRED; + +ALTER TABLE "BillingRecurrence" DROP CONSTRAINT fk_billing_recurrence_domain_history; +ALTER TABLE "BillingRecurrence" ADD CONSTRAINT fk_billing_recurrence_domain_history + FOREIGN KEY (domain_repo_id, domain_history_revision_id) + REFERENCES "DomainHistory"(domain_repo_id, history_revision_id) + DEFERRABLE INITIALLY DEFERRED; + +ALTER TABLE "BillingCancellation" DROP CONSTRAINT fk_billing_cancellation_domain_history; +ALTER TABLE "BillingCancellation" ADD CONSTRAINT fk_billing_cancellation_domain_history + FOREIGN KEY (domain_repo_id, domain_history_revision_id) + REFERENCES "DomainHistory"(domain_repo_id, history_revision_id) + DEFERRABLE INITIALLY DEFERRED; + +ALTER TABLE "PollMessage" DROP CONSTRAINT fk_poll_message_domain_history; +ALTER TABLE "PollMessage" ADD CONSTRAINT fk_poll_message_domain_history + FOREIGN KEY (domain_repo_id, domain_history_revision_id) + REFERENCES "DomainHistory"(domain_repo_id, history_revision_id) + DEFERRABLE INITIALLY DEFERRED; + +ALTER TABLE "PollMessage" DROP CONSTRAINT fk_poll_message_contact_history; +ALTER TABLE "PollMessage" ADD CONSTRAINT fk_poll_message_contact_history + FOREIGN KEY (contact_repo_id, contact_history_revision_id) + REFERENCES "ContactHistory"(contact_repo_id, history_revision_id) + DEFERRABLE INITIALLY DEFERRED; diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index e22fd4e11..a81d39259 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/db/src/main/resources/sql/schema/nomulus.golden.sql @@ -1920,7 +1920,7 @@ ALTER TABLE ONLY public."BillingCancellation" -- ALTER TABLE ONLY public."BillingCancellation" - ADD CONSTRAINT fk_billing_cancellation_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); + ADD CONSTRAINT fk_billing_cancellation_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id) DEFERRABLE INITIALLY DEFERRED; -- @@ -1952,7 +1952,7 @@ ALTER TABLE ONLY public."BillingEvent" -- ALTER TABLE ONLY public."BillingEvent" - ADD CONSTRAINT fk_billing_event_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); + ADD CONSTRAINT fk_billing_event_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id) DEFERRABLE INITIALLY DEFERRED; -- @@ -1968,7 +1968,7 @@ ALTER TABLE ONLY public."BillingEvent" -- ALTER TABLE ONLY public."BillingRecurrence" - ADD CONSTRAINT fk_billing_recurrence_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); + ADD CONSTRAINT fk_billing_recurrence_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id) DEFERRABLE INITIALLY DEFERRED; -- @@ -2216,7 +2216,7 @@ ALTER TABLE ONLY public."HostHistory" -- ALTER TABLE ONLY public."PollMessage" - ADD CONSTRAINT fk_poll_message_contact_history FOREIGN KEY (contact_repo_id, contact_history_revision_id) REFERENCES public."ContactHistory"(contact_repo_id, history_revision_id); + ADD CONSTRAINT fk_poll_message_contact_history FOREIGN KEY (contact_repo_id, contact_history_revision_id) REFERENCES public."ContactHistory"(contact_repo_id, history_revision_id) DEFERRABLE INITIALLY DEFERRED; -- @@ -2232,7 +2232,7 @@ ALTER TABLE ONLY public."PollMessage" -- ALTER TABLE ONLY public."PollMessage" - ADD CONSTRAINT fk_poll_message_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id); + ADD CONSTRAINT fk_poll_message_domain_history FOREIGN KEY (domain_repo_id, domain_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id) DEFERRABLE INITIALLY DEFERRED; --