From 6bcd40f18a09c6a19372d2fbd95890b54ec2ac7d Mon Sep 17 00:00:00 2001 From: guyben Date: Wed, 31 Jan 2018 08:50:45 -0800 Subject: [PATCH] Remove "keepTasks" from ReadDnsQueueAction "keepTasks" is a flag that prevents ReadDnsQueueAction from removing dns-update tasks from the dns-pull queue, while still launching PublishDnsUpdates tasks to update the DNS (meaning these tasks will be updated again in the next ReadDnsQueueAction). I'm not sure what's the purpose of this flag, but given we now allow multiple writers (meaning we can already publish the same DNS multiple times) and given that we can now recover from a bad writer (if a writer doesn't belong to a TLD, we put the dns-updates queued for that writer back into the dns-pull queue) - I suspect we don't need it anymore. Alternative considered: changing this to a "dryRun" flag that won't actually launch PublishDnsUpdates tasks, but will log which tasks it would have launched. Decided against it because we will still need to "own" any task for a significant amount of time if there are many (tens of thousands) tasks in the queue. Hence a "dryRun" will still affect any actual runs for some time. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=183997187 --- java/google/registry/dns/DnsModule.java | 8 ---- .../registry/dns/ReadDnsQueueAction.java | 27 ++++--------- .../registry/dns/ReadDnsQueueActionTest.java | 39 ++++++------------- 3 files changed, 18 insertions(+), 56 deletions(-) diff --git a/java/google/registry/dns/DnsModule.java b/java/google/registry/dns/DnsModule.java index bd98e72f7..5cebb11fe 100644 --- a/java/google/registry/dns/DnsModule.java +++ b/java/google/registry/dns/DnsModule.java @@ -19,8 +19,6 @@ import static google.registry.dns.DnsConstants.DNS_PULL_QUEUE_NAME; import static google.registry.dns.PublishDnsUpdatesAction.PARAM_DNS_WRITER; import static google.registry.dns.PublishDnsUpdatesAction.PARAM_DOMAINS; import static google.registry.dns.PublishDnsUpdatesAction.PARAM_HOSTS; -import static google.registry.dns.ReadDnsQueueAction.PARAM_KEEP_TASKS; -import static google.registry.request.RequestParameters.extractBooleanParameter; import static google.registry.request.RequestParameters.extractEnumParameter; import static google.registry.request.RequestParameters.extractRequiredParameter; import static google.registry.request.RequestParameters.extractSetOfParameters; @@ -76,12 +74,6 @@ public abstract class DnsModule { return extractSetOfParameters(req, PARAM_HOSTS); } - @Provides - @Parameter(PARAM_KEEP_TASKS) - static boolean provideKeepTasks(HttpServletRequest req) { - return extractBooleanParameter(req, PARAM_KEEP_TASKS); - } - @Provides @Parameter("domainOrHostName") static String provideName(HttpServletRequest req) { diff --git a/java/google/registry/dns/ReadDnsQueueAction.java b/java/google/registry/dns/ReadDnsQueueAction.java index 380b52931..8fd64c7d2 100644 --- a/java/google/registry/dns/ReadDnsQueueAction.java +++ b/java/google/registry/dns/ReadDnsQueueAction.java @@ -61,8 +61,6 @@ import org.joda.time.Duration; * * */ @Action( @@ -72,8 +70,6 @@ import org.joda.time.Duration; ) public final class ReadDnsQueueAction implements Runnable { - public static final String PARAM_KEEP_TASKS = "keepTasks"; - private static final String PARAM_JITTER_SECONDS = "jitterSeconds"; private static final Random random = new Random(); private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); @@ -82,7 +78,6 @@ public final class ReadDnsQueueAction implements Runnable { @Inject @Config("dnsWriteLockTimeout") Duration writeLockTimeout; @Inject @Named(DNS_PUBLISH_PUSH_QUEUE_NAME) Queue dnsPublishPushQueue; @Inject @Parameter(PARAM_JITTER_SECONDS) Optional jitterSeconds; - @Inject @Parameter(PARAM_KEEP_TASKS) boolean keepTasks; @Inject DnsQueue dnsQueue; @Inject TaskEnqueuer taskEnqueuer; @Inject ReadDnsQueueAction() {} @@ -187,21 +182,13 @@ public final class ReadDnsQueueAction implements Runnable { } } Set tasksToDelete = difference(ImmutableSet.copyOf(tasks), tasksToKeep); - // In keepTasks mode, never delete any tasks. - if (keepTasks) { - logger.infofmt("Would have deleted %d DNS update tasks.", tasksToDelete.size()); - for (TaskHandle task : tasks) { - dnsQueue.dropTaskLease(task); - } - // Otherwise, either delete or drop the lease of each task. - } else { - logger.infofmt("Deleting %d DNS update tasks.", tasksToDelete.size()); - dnsQueue.deleteTasks(ImmutableList.copyOf(tasksToDelete)); - logger.infofmt("Dropping %d DNS update tasks.", tasksToKeep.size()); - for (TaskHandle task : tasksToKeep) { - dnsQueue.dropTaskLease(task); - } - logger.infofmt("Done processing DNS tasks."); + // Either delete or drop the lease of each task. + logger.infofmt("Deleting %d DNS update tasks.", tasksToDelete.size()); + dnsQueue.deleteTasks(ImmutableList.copyOf(tasksToDelete)); + logger.infofmt("Dropping %d DNS update tasks.", tasksToKeep.size()); + for (TaskHandle task : tasksToKeep) { + dnsQueue.dropTaskLease(task); } + logger.infofmt("Done processing DNS tasks."); } } diff --git a/javatests/google/registry/dns/ReadDnsQueueActionTest.java b/javatests/google/registry/dns/ReadDnsQueueActionTest.java index 09da23eb3..55d72880b 100644 --- a/javatests/google/registry/dns/ReadDnsQueueActionTest.java +++ b/javatests/google/registry/dns/ReadDnsQueueActionTest.java @@ -28,7 +28,6 @@ import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued; import com.google.appengine.api.taskqueue.QueueFactory; import com.google.appengine.api.taskqueue.TaskOptions; import com.google.appengine.api.taskqueue.TaskOptions.Method; -import com.google.appengine.api.taskqueue.dev.QueueStateInfo.TaskStateInfo; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; @@ -39,7 +38,6 @@ import google.registry.model.registry.Registry; import google.registry.model.registry.Registry.TldType; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; -import google.registry.testing.TaskQueueHelper; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.util.Retrier; import google.registry.util.TaskEnqueuer; @@ -99,7 +97,7 @@ public class ReadDnsQueueActionTest { dnsQueue = DnsQueue.create(); } - private void run(boolean keepTasks) throws Exception { + private void run() throws Exception { ReadDnsQueueAction action = new ReadDnsQueueAction(); action.tldUpdateBatchSize = TEST_TLD_UPDATE_BATCH_SIZE; action.writeLockTimeout = Duration.standardSeconds(10); @@ -107,7 +105,6 @@ public class ReadDnsQueueActionTest { action.dnsPublishPushQueue = QueueFactory.getQueue(DNS_PUBLISH_PUSH_QUEUE_NAME); action.taskEnqueuer = new TaskEnqueuer(new Retrier(null, 1)); action.jitterSeconds = Optional.empty(); - action.keepTasks = keepTasks; // Advance the time a little, to ensure that leaseTasks() returns all tasks. clock.setTo(DateTime.now(DateTimeZone.UTC).plusMillis(1)); action.run(); @@ -141,7 +138,7 @@ public class ReadDnsQueueActionTest { dnsQueue.addDomainRefreshTask("domain.com"); dnsQueue.addDomainRefreshTask("domain.net"); dnsQueue.addDomainRefreshTask("domain.example"); - run(false); + run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); assertTasksEnqueued( DNS_PUBLISH_PUSH_QUEUE_NAME, @@ -155,7 +152,7 @@ public class ReadDnsQueueActionTest { dnsQueue.addDomainRefreshTask("domain.com"); dnsQueue.addDomainRefreshTask("domain.net"); dnsQueue.addDomainRefreshTask("domain.example"); - run(false); + run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); assertTldsEnqueuedInPushQueue( ImmutableMultimap.of("com", "comWriter", "net", "netWriter", "example", "exampleWriter")); @@ -169,32 +166,18 @@ public class ReadDnsQueueActionTest { .setDnsWriters(ImmutableSet.of("comWriter", "otherWriter")) .build()); dnsQueue.addDomainRefreshTask("domain.com"); - run(false); + run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); assertTldsEnqueuedInPushQueue(ImmutableMultimap.of("com", "comWriter", "com", "otherWriter")); } - @Test - public void testSuccess_allTldsKeepTasks() throws Exception { - dnsQueue.addDomainRefreshTask("domain.com"); - dnsQueue.addDomainRefreshTask("domain.net"); - dnsQueue.addDomainRefreshTask("domain.example"); - List preexistingTasks = - TaskQueueHelper.getQueueInfo(DNS_PULL_QUEUE_NAME).getTaskInfo(); - run(true); - assertTldsEnqueuedInPushQueue( - ImmutableMultimap.of("com", "comWriter", "net", "netWriter", "example", "exampleWriter")); - // Check that keepTasks was honored and the pull queue tasks are still present in the queue. - assertTasksEnqueued(DNS_PULL_QUEUE_NAME, preexistingTasks); - } - @Test public void testSuccess_oneTldPaused() throws Exception { persistResource(Registry.get("net").asBuilder().setDnsPaused(true).build()); dnsQueue.addDomainRefreshTask("domain.com"); dnsQueue.addDomainRefreshTask("domain.net"); dnsQueue.addDomainRefreshTask("domain.example"); - run(false); + run(); assertTasksEnqueued(DNS_PULL_QUEUE_NAME, new TaskMatcher()); assertTldsEnqueuedInPushQueue( ImmutableMultimap.of("com", "comWriter", "example", "exampleWriter")); @@ -205,7 +188,7 @@ public class ReadDnsQueueActionTest { dnsQueue.addHostRefreshTask("ns1.domain.com"); dnsQueue.addDomainRefreshTask("domain.net"); dnsQueue.addZoneRefreshTask("example"); - run(false); + run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); assertTasksEnqueued( DNS_PUBLISH_PUSH_QUEUE_NAME, @@ -230,10 +213,6 @@ public class ReadDnsQueueActionTest { refreshItemsInTask = 0; } switch (thingType) { - default: - dnsQueue.addDomainRefreshTask(domainName); - task.param("domains", domainName); - break; case 1: getQueue(DNS_PULL_QUEUE_NAME) .add(createRefreshTask("ns1." + domainName, TargetType.HOST)); @@ -244,6 +223,10 @@ public class ReadDnsQueueActionTest { .add(createRefreshTask("ns2." + domainName, TargetType.HOST)); task.param("hosts", "ns2." + domainName); break; + default: + dnsQueue.addDomainRefreshTask(domainName); + task.param("domains", domainName); + break; } // If this task is now full up, wash our hands of it, so that we'll start a new one the // next time through the loop. @@ -254,7 +237,7 @@ public class ReadDnsQueueActionTest { } } } - run(false); + run(); assertNoTasksEnqueued(DNS_PULL_QUEUE_NAME); assertTasksEnqueued(DNS_PUBLISH_PUSH_QUEUE_NAME, expectedTasks); }