Make RefreshDnsForAllDomains SQL-aware

[jianglai]

Also marks a few mapreduce actions as @deprecated as they are no longer
needed in SQL.

---

This change is 

[mindhog]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys and @jianglai)

---

core/src/main/java/google/registry/batch/ResaveAllEppResourcesAction.java, line 57 at r1 (raw file):

    path = "/_dr/task/resaveAllEppResources",
    auth = Auth.AUTH_INTERNAL_OR_ADMIN)
@Deprecated

I'm not sure at-Deprecated has the right semantics for this case as that implies "don't use this" rather then "no longer needed." Can you maybe add a comment to these indicating that these are unnecessary for SQL?

[jianglai]

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @CydeWeys and @mindhog)

---

core/src/main/java/google/registry/batch/ResaveAllEppResourcesAction.java, line 57 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

I'm not sure at-Deprecated has the right semantics for this case as that implies "don't use this" rather then "no longer needed." Can you maybe add a comment to these indicating that these are unnecessary for SQL?

If we were using Java 11 we could have added forRemoval = true to @Deprecated.

[mindhog]

Reviewed 1 of 4 files at r2.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @jianglai, and @mindhog)

---

core/src/main/java/google/registry/backup/DeleteOldCommitLogsAction.java, line 69 at r2 (raw file):

    path = "/_dr/task/deleteOldCommitLogs",
    auth = Auth.AUTH_INTERNAL_OR_ADMIN)
// No longer needed in SQL. Subject to future removal.

Can you add this comment to the other "deprecated" classes, too?

[mindhog]

Reviewed 3 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @CydeWeys)

[CydeWeys]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 103 at r2 (raw file):

      tm().transact(
              () ->
                  jpaTm()

Extract the contents of the transaction to a separate helper function.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 108 at r2 (raw file):

                              + "WHERE tld IN (:tlds) "
                              + "AND deletionTime > :now "
                              + "AND creationTime <= :now",

Why is this necessary? It's not possible for domains to be have been created in the future. There's no such thing as a pending domain create.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

                              response.setPayload(message);
                              response.setStatus(HttpStatus.SC_INTERNAL_SERVER_ERROR);
                              throw t;

The "log and rethrow" pattern is a code smell.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

                              response.setPayload(message);
                              response.setStatus(HttpStatus.SC_INTERNAL_SERVER_ERROR);
                              throw t;

Is this really the correct thing to do? If a single domain's DNS fails to refresh (out of potentially millions), then we terminate and give up?

Note that the mapreduce does NOT terminate on a single failure. It counts and logs failures but does not terminate the entire task.

[jianglai]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @mindhog)

---

core/src/main/java/google/registry/backup/DeleteOldCommitLogsAction.java, line 69 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

Can you add this comment to the other "deprecated" classes, too?

I thought I already did?

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 103 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Extract the contents of the transaction to a separate helper function.

As I said in the other PR, there seems to be limited benefit in doing this. Once the migration is done the run method will just call a helper function, which is only called by the run method, and located somewhere else in the file. Is there a particular reason that you'd prefer this way?

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 108 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Why is this necessary? It's not possible for domains to be have been created in the future. There's no such thing as a pending domain create.

I wondered about this as well. The old mapreduce does this by calling isActive(domain, now). Maybe it's leaving the possibility to run the action at a certain time? But again the time is not injected so it's not possible to provide it. Interestingly we don't always do this: https://cs.opensource.google/nomulus/nomulus/+/master:core/src/main/java/google/registry/model/EppResourceUtils.java;l=70?q=getlinked&ss=nomulus%2Fnomulus

I guess this can be removed to optimize SQL performance.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Is this really the correct thing to do? If a single domain's DNS fails to refresh (out of potentially millions), then we terminate and give up?

Note that the mapreduce does NOT terminate on a single failure. It counts and logs failures but does not terminate the entire task.

Good point.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The "log and rethrow" pattern is a code smell.

Done.

[mindhog]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)

[CydeWeys]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jianglai)

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 103 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

As I said in the other PR, there seems to be limited benefit in doing this. Once the migration is done the run method will just call a helper function, which is only called by the run method, and located somewhere else in the file. Is there a particular reason that you'd prefer this way?

It's marginal here, but the other PR has a function that's even longer and more indented.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 108 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I wondered about this as well. The old mapreduce does this by calling isActive(domain, now). Maybe it's leaving the possibility to run the action at a certain time? But again the time is not injected so it's not possible to provide it. Interestingly we don't always do this: https://cs.opensource.google/nomulus/nomulus/+/master:core/src/main/java/google/registry/model/EppResourceUtils.java;l=70?q=getlinked&ss=nomulus%2Fnomulus

I guess this can be removed to optimize SQL performance.

I can see some actions that require running at a specific point in the past for which you'd want to exclude more recently created domains (RDE is the most obvious use case), but that's not relevant here. Refreshing DNS for all domains is idempotent and that action should generally be a noop anyway unless DNS is out of date. It also doesn't make sense to run a refresh at some point in the past.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Good point.

Can you add a test below where, say, refreshing DNS fails on just 1 of 3, and verify that it still succeeds on the other 2?

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 119 at r3 (raw file):

                                  domainName,
                                  Duration.standardMinutes(random.nextInt(smearMinutes)));
                              logger.atInfo().log("active domain %s refreshed.", domainName);

This is gonna be way too noisy. We don't need a million plus domain names in our logs if we run this action. Note that logging is not analogous to incrementing a mapreduce counter (what happens on success in the other mapreduce).

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 122 at r3 (raw file):

                            } catch (Throwable t) {
                              logger.atSevere().withCause(t).log(
                                  "Error while refreshing DNS for domain %s", domainName);

A more accurate error message is probably "Error while enqueuing DNS refresh of domain %s." Remember that this action isn't actually doing the refreshing, it's just adding the DNS task queue pull entries so that the next time ReadDnsQueueAction runs, it does the refreshing. That could independently still fail even if this succeeds.

[jianglai]

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @mindhog)

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 128 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can you add a test below where, say, refreshing DNS fails on just 1 of 3, and verify that it still succeeds on the other 2?

Done.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 119 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This is gonna be way too noisy. We don't need a million plus domain names in our logs if we run this action. Note that logging is not analogous to incrementing a mapreduce counter (what happens on success in the other mapreduce).

Done.

---

core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java, line 122 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

A more accurate error message is probably "Error while enqueuing DNS refresh of domain %s." Remember that this action isn't actually doing the refreshing, it's just adding the DNS task queue pull entries so that the next time ReadDnsQueueAction runs, it does the refreshing. That could independently still fail even if this succeeds.

Done.

[mindhog]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys)

Reviewer OOO.