From d355da362f3391aeb594dff0c89bc43a63edcaad Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Fri, 18 Feb 2022 20:21:56 -0500 Subject: [PATCH] Make a few quality-of-life improvements in CloudTasksUtils (#1521) * Make a few quality-of-life improvements in CloudTasksUtils 1. Update the method names. There are too many overloaded methods and it is hard to figure out which one does which without checking the javadoc. 2. Added a method in the task matcher to specify the delay time in DateTime, so the caller does not need to convert it to Timestamp. 3. Remove the expilict dependency on a clock when enqueueing a task with delay, the clock is now injected directly into the util instance itself. --- .../backup/CommitLogCheckpointAction.java | 2 +- .../java/google/registry/beam/rde/RdeIO.java | 4 +- .../config/CloudTasksUtilsModule.java | 6 +- .../registry/cron/CommitLogFanoutAction.java | 7 +- .../google/registry/cron/TldFanoutAction.java | 6 +- .../registry/loadtest/LoadTestAction.java | 3 +- .../registry/rde/RdeStagingReducer.java | 4 +- .../google/registry/rde/RdeUploadAction.java | 2 +- .../billing/GenerateInvoicesAction.java | 3 +- .../billing/PublishInvoicesAction.java | 2 +- .../icann/IcannReportingStagingAction.java | 5 +- .../spec11/GenerateSpec11ReportAction.java | 3 +- .../tools/GenerateEscrowDepositCommand.java | 2 +- .../registrar/RegistrarSettingsAction.java | 2 +- .../cron/CommitLogFanoutActionTest.java | 3 +- .../registry/cron/TldFanoutActionTest.java | 3 +- .../billing/GenerateInvoicesActionTest.java | 9 +-- .../IcannReportingStagingActionTest.java | 9 +-- .../GenerateSpec11ReportActionTest.java | 11 ++-- .../registry/testing/CloudTasksHelper.java | 23 +++++-- .../compileClasspath.lockfile | 1 + .../testCompileClasspath.lockfile | 1 + .../compileClasspath.lockfile | 1 + .../testCompileClasspath.lockfile | 1 + .../compileClasspath.lockfile | 2 +- .../testCompileClasspath.lockfile | 2 +- util/build.gradle | 1 + .../compileClasspath.lockfile | 1 + .../testCompileClasspath.lockfile | 1 + .../google/registry/util/CloudTasksUtils.java | 57 +++++++--------- .../registry/util/CloudTasksUtilsTest.java | 65 ++++++++++--------- 31 files changed, 119 insertions(+), 123 deletions(-) diff --git a/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java b/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java index 92273be23..755dc2a71 100644 --- a/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java +++ b/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java @@ -96,7 +96,7 @@ public final class CommitLogCheckpointAction implements Runnable { // Enqueue a diff task between previous and current checkpoints. cloudTasksUtils.enqueue( QUEUE_NAME, - CloudTasksUtils.createPostTask( + cloudTasksUtils.createPostTask( ExportCommitLogDiffAction.PATH, Service.BACKEND.toString(), ImmutableMultimap.of( diff --git a/core/src/main/java/google/registry/beam/rde/RdeIO.java b/core/src/main/java/google/registry/beam/rde/RdeIO.java index 2a6e616f6..41823125f 100644 --- a/core/src/main/java/google/registry/beam/rde/RdeIO.java +++ b/core/src/main/java/google/registry/beam/rde/RdeIO.java @@ -304,7 +304,7 @@ public class RdeIO { if (key.mode() == RdeMode.FULL) { cloudTasksUtils.enqueue( RDE_UPLOAD_QUEUE, - CloudTasksUtils.createPostTask( + cloudTasksUtils.createPostTask( RdeUploadAction.PATH, Service.BACKEND.getServiceId(), ImmutableMultimap.of( @@ -315,7 +315,7 @@ public class RdeIO { } else { cloudTasksUtils.enqueue( BRDA_QUEUE, - CloudTasksUtils.createPostTask( + cloudTasksUtils.createPostTask( BrdaCopyAction.PATH, Service.BACKEND.getServiceId(), ImmutableMultimap.of( diff --git a/core/src/main/java/google/registry/config/CloudTasksUtilsModule.java b/core/src/main/java/google/registry/config/CloudTasksUtilsModule.java index 6e134292b..8b2ad096c 100644 --- a/core/src/main/java/google/registry/config/CloudTasksUtilsModule.java +++ b/core/src/main/java/google/registry/config/CloudTasksUtilsModule.java @@ -21,6 +21,7 @@ import dagger.Module; import dagger.Provides; import google.registry.config.CredentialModule.DefaultCredential; import google.registry.config.RegistryConfig.Config; +import google.registry.util.Clock; import google.registry.util.CloudTasksUtils; import google.registry.util.CloudTasksUtils.GcpCloudTasksClient; import google.registry.util.CloudTasksUtils.SerializableCloudTasksClient; @@ -46,8 +47,9 @@ public abstract class CloudTasksUtilsModule { @Config("projectId") String projectId, @Config("locationId") String locationId, SerializableCloudTasksClient client, - Retrier retrier) { - return new CloudTasksUtils(retrier, projectId, locationId, client); + Retrier retrier, + Clock clock) { + return new CloudTasksUtils(retrier, clock, projectId, locationId, client); } // Provides a supplier instead of using a Dagger @Provider because the latter is not serializable. diff --git a/core/src/main/java/google/registry/cron/CommitLogFanoutAction.java b/core/src/main/java/google/registry/cron/CommitLogFanoutAction.java index 1e5c26fa6..751ec5ae0 100644 --- a/core/src/main/java/google/registry/cron/CommitLogFanoutAction.java +++ b/core/src/main/java/google/registry/cron/CommitLogFanoutAction.java @@ -20,7 +20,6 @@ import google.registry.request.Action; import google.registry.request.Action.Service; import google.registry.request.Parameter; import google.registry.request.auth.Auth; -import google.registry.util.Clock; import google.registry.util.CloudTasksUtils; import java.util.Optional; import javax.inject.Inject; @@ -35,7 +34,6 @@ public final class CommitLogFanoutAction implements Runnable { public static final String BUCKET_PARAM = "bucket"; - @Inject Clock clock; @Inject CloudTasksUtils cloudTasksUtils; @Inject @Parameter("endpoint") String endpoint; @@ -43,18 +41,15 @@ public final class CommitLogFanoutAction implements Runnable { @Inject @Parameter("jitterSeconds") Optional jitterSeconds; @Inject CommitLogFanoutAction() {} - - @Override public void run() { for (int bucketId : CommitLogBucket.getBucketIds()) { cloudTasksUtils.enqueue( queue, - CloudTasksUtils.createPostTask( + cloudTasksUtils.createPostTaskWithJitter( endpoint, Service.BACKEND.toString(), ImmutableMultimap.of(BUCKET_PARAM, Integer.toString(bucketId)), - clock, jitterSeconds)); } } diff --git a/core/src/main/java/google/registry/cron/TldFanoutAction.java b/core/src/main/java/google/registry/cron/TldFanoutAction.java index cf9ced73e..ef8ad074f 100644 --- a/core/src/main/java/google/registry/cron/TldFanoutAction.java +++ b/core/src/main/java/google/registry/cron/TldFanoutAction.java @@ -45,7 +45,6 @@ import google.registry.request.ParameterMap; import google.registry.request.RequestParameters; import google.registry.request.Response; import google.registry.request.auth.Auth; -import google.registry.util.Clock; import google.registry.util.CloudTasksUtils; import java.util.Optional; import java.util.stream.Stream; @@ -98,7 +97,6 @@ public final class TldFanoutAction implements Runnable { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - @Inject Clock clock; @Inject CloudTasksUtils cloudTasksUtils; @Inject Response response; @Inject @Parameter(ENDPOINT_PARAM) String endpoint; @@ -159,7 +157,7 @@ public final class TldFanoutAction implements Runnable { params = ArrayListMultimap.create(params); params.put(RequestParameters.PARAM_TLD, tld); } - return CloudTasksUtils.createPostTask( - endpoint, Service.BACKEND.toString(), params, clock, jitterSeconds); + return cloudTasksUtils.createPostTaskWithJitter( + endpoint, Service.BACKEND.toString(), params, jitterSeconds); } } diff --git a/core/src/main/java/google/registry/loadtest/LoadTestAction.java b/core/src/main/java/google/registry/loadtest/LoadTestAction.java index fb06bf3e1..3b82dac7d 100644 --- a/core/src/main/java/google/registry/loadtest/LoadTestAction.java +++ b/core/src/main/java/google/registry/loadtest/LoadTestAction.java @@ -334,7 +334,8 @@ public class LoadTestAction implements Runnable { tasks.add( Task.newBuilder() .setAppEngineHttpRequest( - CloudTasksUtils.createPostTask( + cloudTasksUtils + .createPostTask( "/_dr/epptool", Service.TOOLS.toString(), ImmutableMultimap.of( diff --git a/core/src/main/java/google/registry/rde/RdeStagingReducer.java b/core/src/main/java/google/registry/rde/RdeStagingReducer.java index 96a8acba1..67e96a303 100644 --- a/core/src/main/java/google/registry/rde/RdeStagingReducer.java +++ b/core/src/main/java/google/registry/rde/RdeStagingReducer.java @@ -233,14 +233,14 @@ public final class RdeStagingReducer extends Reducer matchers = new ArrayList<>(); for (int bucketId : CommitLogBucket.getBucketIds()) { diff --git a/core/src/test/java/google/registry/cron/TldFanoutActionTest.java b/core/src/test/java/google/registry/cron/TldFanoutActionTest.java index 16eef5f61..6e850e569 100644 --- a/core/src/test/java/google/registry/cron/TldFanoutActionTest.java +++ b/core/src/test/java/google/registry/cron/TldFanoutActionTest.java @@ -45,7 +45,7 @@ class TldFanoutActionTest { private static final String ENDPOINT = "/the/servlet"; private static final String QUEUE = "the-queue"; private final FakeResponse response = new FakeResponse(); - private final CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(); + private final CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(new FakeClock()); @RegisterExtension final AppEngineExtension appEngine = @@ -61,7 +61,6 @@ class TldFanoutActionTest { private void run(ImmutableListMultimap params) { TldFanoutAction action = new TldFanoutAction(); - action.clock = new FakeClock(); action.params = params; action.endpoint = ENDPOINT; action.queue = QUEUE; diff --git a/core/src/test/java/google/registry/reporting/billing/GenerateInvoicesActionTest.java b/core/src/test/java/google/registry/reporting/billing/GenerateInvoicesActionTest.java index bf2d44388..3e6f47875 100644 --- a/core/src/test/java/google/registry/reporting/billing/GenerateInvoicesActionTest.java +++ b/core/src/test/java/google/registry/reporting/billing/GenerateInvoicesActionTest.java @@ -23,7 +23,6 @@ import static org.mockito.Mockito.when; import com.google.cloud.tasks.v2.HttpMethod; import com.google.common.net.MediaType; -import com.google.protobuf.util.Timestamps; import google.registry.beam.BeamActionTestBase; import google.registry.model.common.DatabaseMigrationStateSchedule.PrimaryDatabase; import google.registry.reporting.ReportingModule; @@ -83,11 +82,9 @@ class GenerateInvoicesActionTest extends BeamActionTestBase { .param("jobId", "jobid") .param("yearMonth", "2017-10") .scheduleTime( - Timestamps.fromMillis( - clock - .nowUtc() - .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES)) - .getMillis()))); + clock + .nowUtc() + .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES)))); } @TestOfyAndSql diff --git a/core/src/test/java/google/registry/reporting/icann/IcannReportingStagingActionTest.java b/core/src/test/java/google/registry/reporting/icann/IcannReportingStagingActionTest.java index b92757080..392c04974 100644 --- a/core/src/test/java/google/registry/reporting/icann/IcannReportingStagingActionTest.java +++ b/core/src/test/java/google/registry/reporting/icann/IcannReportingStagingActionTest.java @@ -24,7 +24,6 @@ import static org.mockito.Mockito.when; import com.google.cloud.tasks.v2.HttpMethod; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.protobuf.util.Timestamps; import google.registry.bigquery.BigqueryJobFailureException; import google.registry.reporting.icann.IcannReportingModule.ReportType; import google.registry.request.HttpException.BadRequestException; @@ -54,7 +53,8 @@ class IcannReportingStagingActionTest { private YearMonth yearMonth = new YearMonth(2017, 6); private String subdir = "default/dir"; private IcannReportingStagingAction action; - private CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(); + private FakeClock clock = new FakeClock(DateTime.parse("2021-01-02T11:00:00Z")); + private CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(clock); @RegisterExtension final AppEngineExtension appEngine = @@ -77,7 +77,6 @@ class IcannReportingStagingActionTest { action.recipient = new InternetAddress("recipient@example.com"); action.emailService = mock(SendEmailService.class); action.cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); - action.clock = new FakeClock(DateTime.parse("2021-01-02T11:00:00Z")); when(stager.stageReports(yearMonth, subdir, ReportType.ACTIVITY)) .thenReturn(ImmutableList.of("a", "b")); @@ -91,9 +90,7 @@ class IcannReportingStagingActionTest { new TaskMatcher() .url("/_dr/task/icannReportingUpload") .method(HttpMethod.POST) - .scheduleTime( - Timestamps.fromMillis( - action.clock.nowUtc().plus(Duration.standardMinutes(2)).getMillis()))); + .scheduleTime(clock.nowUtc().plus(Duration.standardMinutes(2)))); } @Test diff --git a/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java b/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java index 19b34785b..48eddef0b 100644 --- a/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java +++ b/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java @@ -21,7 +21,6 @@ import static org.mockito.Mockito.when; import com.google.cloud.tasks.v2.HttpMethod; import com.google.common.net.MediaType; -import com.google.protobuf.util.Timestamps; import google.registry.beam.BeamActionTestBase; import google.registry.model.common.DatabaseMigrationStateSchedule.PrimaryDatabase; import google.registry.reporting.ReportingModule; @@ -44,7 +43,7 @@ class GenerateSpec11ReportActionTest extends BeamActionTestBase { AppEngineExtension.builder().withDatastoreAndCloudSql().withTaskQueue().build(); private final FakeClock clock = new FakeClock(DateTime.parse("2018-06-11T12:23:56Z")); - private CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(); + private CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(clock); private CloudTasksUtils cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); private GenerateSpec11ReportAction action; @@ -101,11 +100,9 @@ class GenerateSpec11ReportActionTest extends BeamActionTestBase { .param("jobId", "jobid") .param("date", "2018-06-11") .scheduleTime( - Timestamps.fromMillis( - clock - .nowUtc() - .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES)) - .getMillis()))); + clock + .nowUtc() + .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES)))); } @Test diff --git a/core/src/test/java/google/registry/testing/CloudTasksHelper.java b/core/src/test/java/google/registry/testing/CloudTasksHelper.java index 8df6794cc..215abb80b 100644 --- a/core/src/test/java/google/registry/testing/CloudTasksHelper.java +++ b/core/src/test/java/google/registry/testing/CloudTasksHelper.java @@ -40,6 +40,7 @@ import com.google.common.net.HttpHeaders; import com.google.common.net.MediaType; import com.google.common.truth.Truth8; import com.google.protobuf.Timestamp; +import com.google.protobuf.util.Timestamps; import google.registry.model.ImmutableObject; import google.registry.util.CloudTasksUtils; import google.registry.util.Retrier; @@ -60,6 +61,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import java.util.function.Predicate; import javax.annotation.Nonnull; +import org.joda.time.DateTime; /** * Static utility functions for testing task queues. @@ -92,13 +94,22 @@ public class CloudTasksHelper implements Serializable { private static final String PROJECT_ID = "test-project"; private static final String LOCATION_ID = "test-location"; - private final Retrier retrier = new Retrier(new FakeSleeper(new FakeClock()), 1); private final int instanceId = nextInstanceId.getAndIncrement(); - private final CloudTasksUtils cloudTasksUtils = - new CloudTasksUtils(retrier, PROJECT_ID, LOCATION_ID, new FakeCloudTasksClient()); + private final CloudTasksUtils cloudTasksUtils; + + public CloudTasksHelper(FakeClock clock) { + this.cloudTasksUtils = + new CloudTasksUtils( + new Retrier(new FakeSleeper(clock), 1), + clock, + PROJECT_ID, + LOCATION_ID, + new FakeCloudTasksClient()); + testTasks.put(instanceId, Multimaps.synchronizedListMultimap(LinkedListMultimap.create())); + } public CloudTasksHelper() { - testTasks.put(instanceId, Multimaps.synchronizedListMultimap(LinkedListMultimap.create())); + this(new FakeClock()); } public CloudTasksUtils getTestCloudTasksUtils() { @@ -302,6 +313,10 @@ public class CloudTasksHelper implements Serializable { return this; } + public TaskMatcher scheduleTime(DateTime scheduleTime) { + return scheduleTime(Timestamps.fromMillis(scheduleTime.getMillis())); + } + public TaskMatcher param(String key, String value) { checkNotNull(value, "Test error: A param can never have a null value, so don't assert it"); expected.params.put(key, value); diff --git a/networking/gradle/dependency-locks/compileClasspath.lockfile b/networking/gradle/dependency-locks/compileClasspath.lockfile index 66a2c36f9..9978eb404 100644 --- a/networking/gradle/dependency-locks/compileClasspath.lockfile +++ b/networking/gradle/dependency-locks/compileClasspath.lockfile @@ -30,6 +30,7 @@ com.google.http-client:google-http-client-gson:1.39.2 com.google.http-client:google-http-client:1.39.2 com.google.j2objc:j2objc-annotations:1.3 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.ibm.icu:icu4j:68.2 diff --git a/networking/gradle/dependency-locks/testCompileClasspath.lockfile b/networking/gradle/dependency-locks/testCompileClasspath.lockfile index 1bd027fab..9a537cfaa 100644 --- a/networking/gradle/dependency-locks/testCompileClasspath.lockfile +++ b/networking/gradle/dependency-locks/testCompileClasspath.lockfile @@ -34,6 +34,7 @@ com.google.http-client:google-http-client-gson:1.39.2 com.google.http-client:google-http-client:1.39.2 com.google.j2objc:j2objc-annotations:1.3 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.google.truth:truth:1.1.2 diff --git a/prober/gradle/dependency-locks/compileClasspath.lockfile b/prober/gradle/dependency-locks/compileClasspath.lockfile index 01f3dfe40..c58b8d85f 100644 --- a/prober/gradle/dependency-locks/compileClasspath.lockfile +++ b/prober/gradle/dependency-locks/compileClasspath.lockfile @@ -32,6 +32,7 @@ com.google.http-client:google-http-client:1.39.2 com.google.j2objc:j2objc-annotations:1.3 com.google.monitoring-client:metrics:1.0.7 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.ibm.icu:icu4j:68.2 diff --git a/prober/gradle/dependency-locks/testCompileClasspath.lockfile b/prober/gradle/dependency-locks/testCompileClasspath.lockfile index 794733bd8..8bfd805de 100644 --- a/prober/gradle/dependency-locks/testCompileClasspath.lockfile +++ b/prober/gradle/dependency-locks/testCompileClasspath.lockfile @@ -37,6 +37,7 @@ com.google.j2objc:j2objc-annotations:1.3 com.google.monitoring-client:contrib:1.0.7 com.google.monitoring-client:metrics:1.0.7 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.google.truth:truth:1.1.2 diff --git a/proxy/gradle/dependency-locks/compileClasspath.lockfile b/proxy/gradle/dependency-locks/compileClasspath.lockfile index f284eaf66..e21721057 100644 --- a/proxy/gradle/dependency-locks/compileClasspath.lockfile +++ b/proxy/gradle/dependency-locks/compileClasspath.lockfile @@ -45,7 +45,7 @@ com.google.j2objc:j2objc-annotations:1.3 com.google.monitoring-client:metrics:1.0.7 com.google.monitoring-client:stackdriver:1.0.7 com.google.oauth-client:google-oauth-client:1.31.4 -com.google.protobuf:protobuf-java-util:3.15.3 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.ibm.icu:icu4j:68.2 diff --git a/proxy/gradle/dependency-locks/testCompileClasspath.lockfile b/proxy/gradle/dependency-locks/testCompileClasspath.lockfile index ac3424b30..fe955cdb2 100644 --- a/proxy/gradle/dependency-locks/testCompileClasspath.lockfile +++ b/proxy/gradle/dependency-locks/testCompileClasspath.lockfile @@ -50,7 +50,7 @@ com.google.monitoring-client:contrib:1.0.7 com.google.monitoring-client:metrics:1.0.7 com.google.monitoring-client:stackdriver:1.0.7 com.google.oauth-client:google-oauth-client:1.31.4 -com.google.protobuf:protobuf-java-util:3.15.3 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.google.truth:truth:1.1.2 diff --git a/util/build.gradle b/util/build.gradle index dedbf291e..3fcbfaa2e 100644 --- a/util/build.gradle +++ b/util/build.gradle @@ -30,6 +30,7 @@ dependencies { compile deps['com.google.guava:guava'] compile deps['com.google.http-client:google-http-client'] compile deps['com.google.protobuf:protobuf-java'] + compile deps['com.google.protobuf:protobuf-java-util'] compile deps['com.google.re2j:re2j'] compile deps['com.ibm.icu:icu4j'] compile deps['commons-codec:commons-codec'] diff --git a/util/gradle/dependency-locks/compileClasspath.lockfile b/util/gradle/dependency-locks/compileClasspath.lockfile index 5465e6896..7de975066 100644 --- a/util/gradle/dependency-locks/compileClasspath.lockfile +++ b/util/gradle/dependency-locks/compileClasspath.lockfile @@ -29,6 +29,7 @@ com.google.http-client:google-http-client-gson:1.39.2 com.google.http-client:google-http-client:1.39.2 com.google.j2objc:j2objc-annotations:1.3 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.ibm.icu:icu4j:68.2 diff --git a/util/gradle/dependency-locks/testCompileClasspath.lockfile b/util/gradle/dependency-locks/testCompileClasspath.lockfile index 66bd3f9cb..7aaa22c17 100644 --- a/util/gradle/dependency-locks/testCompileClasspath.lockfile +++ b/util/gradle/dependency-locks/testCompileClasspath.lockfile @@ -35,6 +35,7 @@ com.google.http-client:google-http-client-gson:1.39.2 com.google.http-client:google-http-client:1.39.2 com.google.j2objc:j2objc-annotations:1.3 com.google.oauth-client:google-oauth-client:1.31.4 +com.google.protobuf:protobuf-java-util:3.17.3 com.google.protobuf:protobuf-java:3.17.3 com.google.re2j:re2j:1.6 com.google.truth:truth:1.1.2 diff --git a/util/src/main/java/google/registry/util/CloudTasksUtils.java b/util/src/main/java/google/registry/util/CloudTasksUtils.java index 7cf144ead..7e4046928 100644 --- a/util/src/main/java/google/registry/util/CloudTasksUtils.java +++ b/util/src/main/java/google/registry/util/CloudTasksUtils.java @@ -35,10 +35,9 @@ import com.google.common.net.HttpHeaders; import com.google.common.net.MediaType; import com.google.common.net.UrlEscapers; import com.google.protobuf.ByteString; -import com.google.protobuf.Timestamp; +import com.google.protobuf.util.Timestamps; import java.io.Serializable; import java.nio.charset.StandardCharsets; -import java.time.Instant; import java.util.Arrays; import java.util.Optional; import java.util.Random; @@ -53,13 +52,19 @@ public class CloudTasksUtils implements Serializable { private static final Random random = new Random(); private final Retrier retrier; + private final Clock clock; private final String projectId; private final String locationId; private final SerializableCloudTasksClient client; public CloudTasksUtils( - Retrier retrier, String projectId, String locationId, SerializableCloudTasksClient client) { + Retrier retrier, + Clock clock, + String projectId, + String locationId, + SerializableCloudTasksClient client) { this.retrier = retrier; + this.clock = clock; this.projectId = projectId; this.locationId = locationId; this.client = client; @@ -102,7 +107,7 @@ public class CloudTasksUtils implements Serializable { * href=ttps://cloud.google.com/appengine/docs/standard/java/taskqueue/push/creating-tasks#target>Specifyinig * the worker service */ - private static Task createTask( + private Task createTask( String path, HttpMethod method, String service, Multimap params) { checkArgument( path != null && !path.isEmpty() && path.charAt(0) == '/', @@ -151,29 +156,26 @@ public class CloudTasksUtils implements Serializable { * needs to be explicitly specified. * @param params a multi-map of URL query parameters. Duplicate keys are saved as is, and it is up * to the server to process the duplicate keys. - * @param clock a source of time. * @param jitterSeconds the number of seconds that a task is randomly delayed up to. * @return the enqueued task. * @see Specifyinig * the worker service */ - private static Task createTask( + private Task createTaskWithJitter( String path, HttpMethod method, String service, Multimap params, - Clock clock, Optional jitterSeconds) { if (!jitterSeconds.isPresent() || jitterSeconds.get() <= 0) { return createTask(path, method, service, params); } - return createTask( + return createTaskWithDelay( path, method, service, params, - clock, Duration.millis(random.nextInt((int) SECONDS.toMillis(jitterSeconds.get())))); } @@ -188,76 +190,67 @@ public class CloudTasksUtils implements Serializable { * needs to be explicitly specified. * @param params a multi-map of URL query parameters. Duplicate keys are saved as is, and it is up * to the server to process the duplicate keys. - * @param clock a source of time. * @param delay the amount of time that a task needs to delayed for. * @return the enqueued task. * @see Specifyinig * the worker service */ - private static Task createTask( + private Task createTaskWithDelay( String path, HttpMethod method, String service, Multimap params, - Clock clock, Duration delay) { if (delay.isEqual(Duration.ZERO)) { return createTask(path, method, service, params); } checkArgument(delay.isLongerThan(Duration.ZERO), "Negative duration is not supported."); - Instant scheduleTime = Instant.ofEpochMilli(clock.nowUtc().getMillis() + delay.getMillis()); return Task.newBuilder(createTask(path, method, service, params)) - .setScheduleTime( - Timestamp.newBuilder() - .setSeconds(scheduleTime.getEpochSecond()) - .setNanos(scheduleTime.getNano()) - .build()) + .setScheduleTime(Timestamps.fromMillis(clock.nowUtc().plus(delay).getMillis())) .build(); } - public static Task createPostTask(String path, String service, Multimap params) { + public Task createPostTask(String path, String service, Multimap params) { return createTask(path, HttpMethod.POST, service, params); } - public static Task createGetTask(String path, String service, Multimap params) { + public Task createGetTask(String path, String service, Multimap params) { return createTask(path, HttpMethod.GET, service, params); } /** * Create a {@link Task} via HTTP.POST that will be randomly delayed up to {@code jitterSeconds}. */ - public static Task createPostTask( + public Task createPostTaskWithJitter( String path, String service, Multimap params, - Clock clock, Optional jitterSeconds) { - return createTask(path, HttpMethod.POST, service, params, clock, jitterSeconds); + return createTaskWithJitter(path, HttpMethod.POST, service, params, jitterSeconds); } /** * Create a {@link Task} via HTTP.GET that will be randomly delayed up to {@code jitterSeconds}. */ - public static Task createGetTask( + public Task createGetTaskWithJitter( String path, String service, Multimap params, - Clock clock, Optional jitterSeconds) { - return createTask(path, HttpMethod.GET, service, params, clock, jitterSeconds); + return createTaskWithJitter(path, HttpMethod.GET, service, params, jitterSeconds); } /** Create a {@link Task} via HTTP.POST that will be delayed for {@code delay}. */ - public static Task createPostTask( - String path, String service, Multimap params, Clock clock, Duration delay) { - return createTask(path, HttpMethod.POST, service, params, clock, delay); + public Task createPostTaskWithDelay( + String path, String service, Multimap params, Duration delay) { + return createTaskWithDelay(path, HttpMethod.POST, service, params, delay); } /** Create a {@link Task} via HTTP.GET that will be delayed for {@code delay}. */ - public static Task createGetTask( - String path, String service, Multimap params, Clock clock, Duration delay) { - return createTask(path, HttpMethod.GET, service, params, clock, delay); + public Task createGetTaskWithDelay( + String path, String service, Multimap params, Duration delay) { + return createTaskWithDelay(path, HttpMethod.GET, service, params, delay); } public abstract static class SerializableCloudTasksClient implements Serializable { diff --git a/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java b/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java index 44b12338c..c1cf29aba 100644 --- a/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java +++ b/util/src/test/java/google/registry/util/CloudTasksUtilsTest.java @@ -43,10 +43,10 @@ public class CloudTasksUtilsTest { // Use a LinkedListMultimap to preserve order of the inserted entries for assertion. private final LinkedListMultimap params = LinkedListMultimap.create(); private final SerializableCloudTasksClient mockClient = mock(SerializableCloudTasksClient.class); + private final FakeClock clock = new FakeClock(DateTime.parse("2021-11-08")); private final CloudTasksUtils cloudTasksUtils = new CloudTasksUtils( - new Retrier(new FakeSleeper(new FakeClock()), 1), "project", "location", mockClient); - private final Clock clock = new FakeClock(DateTime.parse("2021-11-08")); + new Retrier(new FakeSleeper(clock), 1), clock, "project", "location", mockClient); @BeforeEach void beforeEach() { @@ -59,7 +59,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks() { - Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", params); + Task task = cloudTasksUtils.createGetTask("/the/path", "myservice", params); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -70,7 +70,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks() { - Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", params); + Task task = cloudTasksUtils.createPostTask("/the/path", "myservice", params); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -84,7 +84,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withNullParams() { - Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", null); + Task task = cloudTasksUtils.createGetTask("/the/path", "myservice", null); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -94,7 +94,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withNullParams() { - Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", null); + Task task = cloudTasksUtils.createPostTask("/the/path", "myservice", null); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -105,7 +105,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withEmptyParams() { - Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", ImmutableMultimap.of()); + Task task = cloudTasksUtils.createGetTask("/the/path", "myservice", ImmutableMultimap.of()); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -115,7 +115,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withEmptyParams() { - Task task = CloudTasksUtils.createPostTask("/the/path", "myservice", ImmutableMultimap.of()); + Task task = cloudTasksUtils.createPostTask("/the/path", "myservice", ImmutableMultimap.of()); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -128,7 +128,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withJitterSeconds() { Task task = - CloudTasksUtils.createGetTask("/the/path", "myservice", params, clock, Optional.of(100)); + cloudTasksUtils.createGetTaskWithJitter("/the/path", "myservice", params, Optional.of(100)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -147,7 +147,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withJitterSeconds() { Task task = - CloudTasksUtils.createPostTask("/the/path", "myservice", params, clock, Optional.of(1)); + cloudTasksUtils.createPostTaskWithJitter("/the/path", "myservice", params, Optional.of(1)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -169,7 +169,8 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withEmptyJitterSeconds() { Task task = - CloudTasksUtils.createPostTask("/the/path", "myservice", params, clock, Optional.empty()); + cloudTasksUtils.createPostTaskWithJitter( + "/the/path", "myservice", params, Optional.empty()); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -184,7 +185,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withEmptyJitterSeconds() { Task task = - CloudTasksUtils.createGetTask("/the/path", "myservice", params, clock, Optional.empty()); + cloudTasksUtils.createGetTaskWithJitter("/the/path", "myservice", params, Optional.empty()); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -196,7 +197,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withZeroJitterSeconds() { Task task = - CloudTasksUtils.createPostTask("/the/path", "myservice", params, clock, Optional.of(0)); + cloudTasksUtils.createPostTaskWithJitter("/the/path", "myservice", params, Optional.of(0)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -211,7 +212,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withZeroJitterSeconds() { Task task = - CloudTasksUtils.createGetTask("/the/path", "myservice", params, clock, Optional.of(0)); + cloudTasksUtils.createGetTaskWithJitter("/the/path", "myservice", params, Optional.of(0)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -223,8 +224,8 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withDelay() { Task task = - CloudTasksUtils.createGetTask( - "/the/path", "myservice", params, clock, Duration.standardMinutes(10)); + cloudTasksUtils.createGetTaskWithDelay( + "/the/path", "myservice", params, Duration.standardMinutes(10)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -237,8 +238,8 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createPostTasks_withDelay() { Task task = - CloudTasksUtils.createPostTask( - "/the/path", "myservice", params, clock, Duration.standardMinutes(10)); + cloudTasksUtils.createPostTaskWithDelay( + "/the/path", "myservice", params, Duration.standardMinutes(10)); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -258,8 +259,8 @@ public class CloudTasksUtilsTest { assertThrows( IllegalArgumentException.class, () -> - CloudTasksUtils.createGetTask( - "/the/path", "myservice", params, clock, Duration.standardMinutes(-10))); + cloudTasksUtils.createGetTaskWithDelay( + "/the/path", "myservice", params, Duration.standardMinutes(-10))); assertThat(thrown).hasMessageThat().isEqualTo("Negative duration is not supported."); } @@ -269,15 +270,15 @@ public class CloudTasksUtilsTest { assertThrows( IllegalArgumentException.class, () -> - CloudTasksUtils.createGetTask( - "/the/path", "myservice", params, clock, Duration.standardMinutes(-10))); + cloudTasksUtils.createGetTaskWithDelay( + "/the/path", "myservice", params, Duration.standardMinutes(-10))); assertThat(thrown).hasMessageThat().isEqualTo("Negative duration is not supported."); } @Test void testSuccess_createPostTasks_withZeroDelay() { Task task = - CloudTasksUtils.createPostTask("/the/path", "myservice", params, clock, Duration.ZERO); + cloudTasksUtils.createPostTaskWithDelay("/the/path", "myservice", params, Duration.ZERO); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.POST); assertThat(task.getAppEngineHttpRequest().getRelativeUri()).isEqualTo("/the/path"); assertThat(task.getAppEngineHttpRequest().getAppEngineRouting().getService()) @@ -292,7 +293,7 @@ public class CloudTasksUtilsTest { @Test void testSuccess_createGetTasks_withZeroDelay() { Task task = - CloudTasksUtils.createGetTask("/the/path", "myservice", params, clock, Duration.ZERO); + cloudTasksUtils.createGetTaskWithDelay("/the/path", "myservice", params, Duration.ZERO); assertThat(task.getAppEngineHttpRequest().getHttpMethod()).isEqualTo(HttpMethod.GET); assertThat(task.getAppEngineHttpRequest().getRelativeUri()) .isEqualTo("/the/path?key1=val1&key2=val2&key1=val3"); @@ -305,26 +306,26 @@ public class CloudTasksUtilsTest { void testFailure_illegalPath() { assertThrows( IllegalArgumentException.class, - () -> CloudTasksUtils.createPostTask("the/path", "myservice", params)); + () -> cloudTasksUtils.createPostTask("the/path", "myservice", params)); assertThrows( IllegalArgumentException.class, - () -> CloudTasksUtils.createPostTask(null, "myservice", params)); + () -> cloudTasksUtils.createPostTask(null, "myservice", params)); assertThrows( IllegalArgumentException.class, - () -> CloudTasksUtils.createPostTask("", "myservice", params)); + () -> cloudTasksUtils.createPostTask("", "myservice", params)); } @Test void testSuccess_enqueueTask() { - Task task = CloudTasksUtils.createGetTask("/the/path", "myservice", params); + Task task = cloudTasksUtils.createGetTask("/the/path", "myservice", params); cloudTasksUtils.enqueue("test-queue", task); verify(mockClient).enqueue("project", "location", "test-queue", task); } @Test void testSuccess_enqueueTasks_varargs() { - Task task1 = CloudTasksUtils.createGetTask("/the/path", "myservice", params); - Task task2 = CloudTasksUtils.createGetTask("/other/path", "yourservice", params); + Task task1 = cloudTasksUtils.createGetTask("/the/path", "myservice", params); + Task task2 = cloudTasksUtils.createGetTask("/other/path", "yourservice", params); cloudTasksUtils.enqueue("test-queue", task1, task2); verify(mockClient).enqueue("project", "location", "test-queue", task1); verify(mockClient).enqueue("project", "location", "test-queue", task2); @@ -332,8 +333,8 @@ public class CloudTasksUtilsTest { @Test void testSuccess_enqueueTasks_iterable() { - Task task1 = CloudTasksUtils.createGetTask("/the/path", "myservice", params); - Task task2 = CloudTasksUtils.createGetTask("/other/path", "yourservice", params); + Task task1 = cloudTasksUtils.createGetTask("/the/path", "myservice", params); + Task task2 = cloudTasksUtils.createGetTask("/other/path", "yourservice", params); cloudTasksUtils.enqueue("test-queue", ImmutableList.of(task1, task2)); verify(mockClient).enqueue("project", "location", "test-queue", task1); verify(mockClient).enqueue("project", "location", "test-queue", task2);