From a16794e2af28d5fbef50bdf4fbdf57e920be8fbe Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Wed, 10 Apr 2024 15:58:24 -0400 Subject: [PATCH] Run BSA Validate without lock (#2399) As a read-only action that tolerates staleness, locking is unnecessary. This should help with the lock contention we are observing. Also reduces the number of VM instances provisioned for BSA and increase the idle timeout. This should reduce invocation delay. Longer delay may cause AppEngine to return `Timeout` status to Cloud Scheduler even though the cron job succeeds. --- .../registry/bsa/BsaValidateAction.java | 16 ++------ .../production/bsa/WEB-INF/appengine-web.xml | 4 +- .../env/sandbox/bsa/WEB-INF/appengine-web.xml | 4 +- .../registry/bsa/BsaValidateActionTest.java | 38 ++----------------- 4 files changed, 11 insertions(+), 51 deletions(-) diff --git a/core/src/main/java/google/registry/bsa/BsaValidateAction.java b/core/src/main/java/google/registry/bsa/BsaValidateAction.java index 4bbbf8771..e98b3d2aa 100644 --- a/core/src/main/java/google/registry/bsa/BsaValidateAction.java +++ b/core/src/main/java/google/registry/bsa/BsaValidateAction.java @@ -81,7 +81,6 @@ public class BsaValidateAction implements Runnable { private final int transactionBatchSize; private final Duration maxStaleness; private final Clock clock; - private final BsaLock bsaLock; private final Response response; @Inject @@ -92,7 +91,6 @@ public class BsaValidateAction implements Runnable { @Config("bsaTxnBatchSize") int transactionBatchSize, @Config("bsaValidationMaxStaleness") Duration maxStaleness, Clock clock, - BsaLock bsaLock, Response response) { this.gcsClient = gcsClient; this.idnChecker = idnChecker; @@ -100,18 +98,13 @@ public class BsaValidateAction implements Runnable { this.transactionBatchSize = transactionBatchSize; this.maxStaleness = maxStaleness; this.clock = clock; - this.bsaLock = bsaLock; this.response = response; } @Override public void run() { try { - if (!bsaLock.executeWithLock(this::runWithinLock)) { - String message = "BSA validation did not run: another BSA related task is running"; - logger.atInfo().log("%s.", message); - emailSender.sendNotification(message, /* body= */ ""); - } + validate(); } catch (Throwable throwable) { logger.atWarning().withCause(throwable).log("Failed to validate block lists."); emailSender.sendNotification("BSA validation aborted", getStackTraceAsString(throwable)); @@ -121,15 +114,15 @@ public class BsaValidateAction implements Runnable { response.setStatus(SC_OK); } - /** Executes the validation action while holding the BSA lock. */ - Void runWithinLock() { + /** Performs validation of BSA data in the database. */ + void validate() { Optional downloadJobName = bsaQuery(DownloadScheduler::fetchMostRecentDownloadJobIdIfCompleted); if (downloadJobName.isEmpty()) { logger.atInfo().log("Cannot validate: block list downloads not found."); emailSender.sendNotification( "BSA validation does not run: block list downloads not found", ""); - return null; + return; } logger.atInfo().log("Validating BSA with latest download: %s", downloadJobName.get()); @@ -148,7 +141,6 @@ public class BsaValidateAction implements Runnable { emailValidationResults(resultSummary, downloadJobName.get(), errors); } logger.atInfo().log("%s (latest download: %s)", resultSummary, downloadJobName.get()); - return null; } void emailValidationResults(String subject, String jobName, ImmutableList results) { diff --git a/core/src/main/java/google/registry/env/production/bsa/WEB-INF/appengine-web.xml b/core/src/main/java/google/registry/env/production/bsa/WEB-INF/appengine-web.xml index 10eea0a62..873156a6b 100644 --- a/core/src/main/java/google/registry/env/production/bsa/WEB-INF/appengine-web.xml +++ b/core/src/main/java/google/registry/env/production/bsa/WEB-INF/appengine-web.xml @@ -7,8 +7,8 @@ true B4_1G - 100 - 10m + 3 + 60m diff --git a/core/src/main/java/google/registry/env/sandbox/bsa/WEB-INF/appengine-web.xml b/core/src/main/java/google/registry/env/sandbox/bsa/WEB-INF/appengine-web.xml index d76251b55..8c5a849cf 100644 --- a/core/src/main/java/google/registry/env/sandbox/bsa/WEB-INF/appengine-web.xml +++ b/core/src/main/java/google/registry/env/sandbox/bsa/WEB-INF/appengine-web.xml @@ -7,8 +7,8 @@ true B4 - 100 - 10m + 3 + 60m diff --git a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java index 987030aaf..350e6a516 100644 --- a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java +++ b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java @@ -31,6 +31,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; @@ -56,7 +57,6 @@ import google.registry.testing.FakeClock; import google.registry.tldconfig.idn.IdnTableEnum; import google.registry.util.EmailMessage; import java.util.Optional; -import java.util.concurrent.Callable; import java.util.stream.Stream; import javax.mail.internet.InternetAddress; import org.joda.time.DateTime; @@ -90,8 +90,6 @@ public class BsaValidateActionTest { @Mock Response response; - @Mock private BsaLock bsaLock; - @Mock private InternetAddress emailRecipient; @Captor ArgumentCaptor emailCaptor = ArgumentCaptor.forClass(EmailMessage.class); @@ -112,7 +110,6 @@ public class BsaValidateActionTest { /* transactionBatchSize= */ 500, MAX_STALENESS, fakeClock, - bsaLock, response); createTld("app"); } @@ -357,22 +354,11 @@ public class BsaValidateActionTest { "Registered domain registered-missing.app missing or not recorded as REGISTERED"); } - @Test - void notificationSent_cannotAcquireLock() { - when(bsaLock.executeWithLock(any())).thenReturn(false); - action.run(); - verify(gmailClient, times(1)) - .sendEmail( - EmailMessage.create( - "BSA validation did not run: another BSA related task is running", - "", - emailRecipient)); - } - @Test void notificationSent_abortedByException() { + action = spy(action); RuntimeException throwable = new RuntimeException("Error"); - when(bsaLock.executeWithLock(any())).thenThrow(throwable); + doThrow(throwable).when(action).validate(); action.run(); verify(gmailClient, times(1)) .sendEmail( @@ -382,12 +368,6 @@ public class BsaValidateActionTest { @Test void notificationSent_noDownloads() { - when(bsaLock.executeWithLock(any())) - .thenAnswer( - args -> { - args.getArgument(0, Callable.class).call(); - return true; - }); action.run(); verify(gmailClient, times(1)) .sendEmail( @@ -397,12 +377,6 @@ public class BsaValidateActionTest { @Test void notificationSent_withValidationError() { - when(bsaLock.executeWithLock(any())) - .thenAnswer( - args -> { - args.getArgument(0, Callable.class).call(); - return true; - }); persistDownloadSchedule(DownloadStage.DONE); action = spy(action); doReturn(ImmutableList.of("Error line 1.", "Error line 2")) @@ -420,12 +394,6 @@ public class BsaValidateActionTest { @Test void notification_notSent_WhenNoError() { - when(bsaLock.executeWithLock(any())) - .thenAnswer( - args -> { - args.getArgument(0, Callable.class).call(); - return true; - }); persistDownloadSchedule(DownloadStage.DONE); action = spy(action); doReturn(ImmutableList.of()).when(action).checkBsaLabels(anyString());