From 992d1c13490af96598fa553390680c5dbe9375ba Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Wed, 8 Nov 2023 13:47:37 -0500 Subject: [PATCH] Reduce the QPS of the refresh DNS for all domains action (#2212) This also adds a targeted QPS as a parameter in case we need to manually bump it up (or down) for some reason without having to make code changes and re-deploy. --- .../server/RefreshDnsForAllDomainsAction.java | 32 +++++++++++++------ .../tools/server/ToolsServerModule.java | 6 ++++ .../RefreshDnsForAllDomainsActionTest.java | 9 +++--- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java b/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java index 22a5d399a..7512c31a3 100644 --- a/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java +++ b/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java @@ -58,13 +58,25 @@ public class RefreshDnsForAllDomainsAction implements Runnable { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + /** The number of DNS updates to enqueue per transaction. */ private static final int DEFAULT_BATCH_SIZE = 250; + /** + * The default number of DNS updates it is safe to execute per minute. + * + *

This is mostly a guess based on existing system performance, but the point is to be on the + * safe side and not cause contention with ongoing DNS updates from clients. + */ + private static final int DEFAULT_REFRESH_QPS = 7; + private final Response response; private final ImmutableSet tlds; // Recommended value for batch size is between 200 and 500 private final int batchSize; + + private final int refreshQps; + private final Random random; @Inject @@ -72,10 +84,12 @@ public class RefreshDnsForAllDomainsAction implements Runnable { Response response, @Parameter(PARAM_TLDS) ImmutableSet tlds, @Parameter("batchSize") Optional batchSize, + @Parameter("refreshQps") Optional refreshQps, Random random) { this.response = response; this.tlds = tlds; this.batchSize = batchSize.orElse(DEFAULT_BATCH_SIZE); + this.refreshQps = refreshQps.orElse(DEFAULT_REFRESH_QPS); this.random = random; } @@ -83,7 +97,7 @@ public class RefreshDnsForAllDomainsAction implements Runnable { public void run() { assertTldsExist(tlds); checkArgument(batchSize > 0, "Must specify a positive number for batch size"); - int smearMinutes = tm().transact(this::calculateSmearMinutes, TRANSACTION_REPEATABLE_READ); + Duration smear = tm().transact(this::calculateSmear, TRANSACTION_REPEATABLE_READ); ImmutableList domainsBatch; @Nullable String lastInPreviousBatch = null; @@ -91,17 +105,16 @@ public class RefreshDnsForAllDomainsAction implements Runnable { Optional lastInPreviousBatchOpt = Optional.ofNullable(lastInPreviousBatch); domainsBatch = tm().transact( - () -> refreshBatch(lastInPreviousBatchOpt, smearMinutes), - TRANSACTION_REPEATABLE_READ); + () -> refreshBatch(lastInPreviousBatchOpt, smear), TRANSACTION_REPEATABLE_READ); lastInPreviousBatch = domainsBatch.isEmpty() ? null : getLast(domainsBatch); } while (domainsBatch.size() == batchSize); } /** - * Calculates the number of smear minutes to enqueue refreshes so that the DNS queue does not get + * Calculates the smear duration to enqueue refreshes so that the DNS queue does not get * overloaded. */ - private int calculateSmearMinutes() { + private Duration calculateSmear() { Long activeDomains = tm().query( "SELECT COUNT(*) FROM Domain WHERE tld IN (:tlds) AND deletionTime = :endOfTime", @@ -109,7 +122,7 @@ public class RefreshDnsForAllDomainsAction implements Runnable { .setParameter("tlds", tlds) .setParameter("endOfTime", END_OF_TIME) .getSingleResult(); - return Math.max(activeDomains.intValue() / 1000, 1); + return Duration.standardSeconds(Math.max(activeDomains / refreshQps, 1)); } private ImmutableList getBatch(Optional lastInPreviousBatch) { @@ -127,11 +140,12 @@ public class RefreshDnsForAllDomainsAction implements Runnable { } @VisibleForTesting - ImmutableList refreshBatch(Optional lastInPreviousBatch, int smearMinutes) { + ImmutableList refreshBatch(Optional lastInPreviousBatch, Duration smear) { ImmutableList domainBatch = getBatch(lastInPreviousBatch); try { - // Smear the task execution time over the next N minutes. - requestDomainDnsRefresh(domainBatch, Duration.standardMinutes(random.nextInt(smearMinutes))); + // Smear the task execution time over the next N seconds. + requestDomainDnsRefresh( + domainBatch, Duration.standardSeconds(random.nextInt((int) smear.getStandardSeconds()))); } catch (Throwable t) { logger.atSevere().withCause(t).log("Error while enqueuing DNS refresh batch"); response.setStatus(HttpStatus.SC_OK); diff --git a/core/src/main/java/google/registry/tools/server/ToolsServerModule.java b/core/src/main/java/google/registry/tools/server/ToolsServerModule.java index a85b3c10f..4159e0aa0 100644 --- a/core/src/main/java/google/registry/tools/server/ToolsServerModule.java +++ b/core/src/main/java/google/registry/tools/server/ToolsServerModule.java @@ -81,4 +81,10 @@ public class ToolsServerModule { static Optional provideBatchSize(HttpServletRequest req) { return extractOptionalIntParameter(req, "batchSize"); } + + @Provides + @Parameter("refreshQps") + static Optional provideRefreshQps(HttpServletRequest req) { + return extractOptionalIntParameter(req, "refreshQps"); + } } diff --git a/core/src/test/java/google/registry/tools/server/RefreshDnsForAllDomainsActionTest.java b/core/src/test/java/google/registry/tools/server/RefreshDnsForAllDomainsActionTest.java index 61fbd18af..e1c643893 100644 --- a/core/src/test/java/google/registry/tools/server/RefreshDnsForAllDomainsActionTest.java +++ b/core/src/test/java/google/registry/tools/server/RefreshDnsForAllDomainsActionTest.java @@ -35,6 +35,7 @@ import google.registry.testing.FakeResponse; import java.util.Optional; import java.util.Random; import org.joda.time.DateTime; +import org.joda.time.Duration; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -55,7 +56,7 @@ public class RefreshDnsForAllDomainsActionTest { createTld("bar"); action = new RefreshDnsForAllDomainsAction( - response, ImmutableSet.of("bar"), Optional.of(10), new Random()); + response, ImmutableSet.of("bar"), Optional.of(10), Optional.empty(), new Random()); } @Test @@ -74,9 +75,9 @@ public class RefreshDnsForAllDomainsActionTest { // Set batch size to 1 since each batch will be enqueud at the same time action = new RefreshDnsForAllDomainsAction( - response, ImmutableSet.of("bar"), Optional.of(1), new Random()); - tm().transact(() -> action.refreshBatch(Optional.empty(), 1000)); - tm().transact(() -> action.refreshBatch(Optional.empty(), 1000)); + response, ImmutableSet.of("bar"), Optional.of(1), Optional.of(7), new Random()); + tm().transact(() -> action.refreshBatch(Optional.empty(), Duration.standardMinutes(1000))); + tm().transact(() -> action.refreshBatch(Optional.empty(), Duration.standardMinutes(1000))); ImmutableList refreshRequests = tm().transact( () ->