diff --git a/java/google/registry/batch/MapreduceEntityCleanupAction.java b/java/google/registry/batch/MapreduceEntityCleanupAction.java index 6c8339b59..df71c7ad4 100644 --- a/java/google/registry/batch/MapreduceEntityCleanupAction.java +++ b/java/google/registry/batch/MapreduceEntityCleanupAction.java @@ -14,10 +14,12 @@ package google.registry.batch; +import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; +import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; + import com.google.appengine.api.datastore.DatastoreService; import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; -import com.google.common.net.MediaType; import google.registry.batch.MapreduceEntityCleanupUtil.EligibleJobResults; import google.registry.mapreduce.MapreduceRunner; import google.registry.request.Action; @@ -79,6 +81,8 @@ public class MapreduceEntityCleanupAction implements Runnable { "Do not specify both a job ID and a number of jobs to delete"; private static final String ERROR_BOTH_JOB_ID_AND_DAYS_OLD = "Do not specify both a job ID and a days old threshold"; + private static final String ERROR_NON_POSITIVE_JOBS_TO_DELETE = + "Do not specify a non-positive integer for the number of jobs to delete"; private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); @@ -116,7 +120,7 @@ public class MapreduceEntityCleanupAction implements Runnable { @Override public void run() { - response.setContentType(MediaType.PLAIN_TEXT_UTF_8); + response.setContentType(PLAIN_TEXT_UTF_8); if (jobId.isPresent()) { runWithJobId(); } else { @@ -124,26 +128,27 @@ public class MapreduceEntityCleanupAction implements Runnable { } } - private void logSevereAndSetPayload(String message) { + private void handleBadRequest(String message) { logger.severe(message); response.setPayload(message); + response.setStatus(SC_BAD_REQUEST); } /** Delete the job with the specified job ID, checking for conflicting parameters. */ private void runWithJobId() { if (jobName.isPresent()) { - logSevereAndSetPayload(ERROR_BOTH_JOB_ID_AND_NAME); + handleBadRequest(ERROR_BOTH_JOB_ID_AND_NAME); return; } if (numJobsToDelete.isPresent()) { - logSevereAndSetPayload(ERROR_BOTH_JOB_ID_AND_NUMBER_OF_JOBS); + handleBadRequest(ERROR_BOTH_JOB_ID_AND_NUMBER_OF_JOBS); return; } if (daysOld.isPresent()) { - logSevereAndSetPayload(ERROR_BOTH_JOB_ID_AND_DAYS_OLD); + handleBadRequest(ERROR_BOTH_JOB_ID_AND_DAYS_OLD); return; } - response.setPayload(requestDeletion(ImmutableSet.of(jobId.get()), true /* generatePayload */)); + response.setPayload(requestDeletion(ImmutableSet.of(jobId.get()), true /* verbose */)); } /** @@ -151,60 +156,55 @@ public class MapreduceEntityCleanupAction implements Runnable { * which are old enough. */ private void runWithoutJobId() { + if (numJobsToDelete.isPresent() && numJobsToDelete.get() <= 0) { + handleBadRequest(ERROR_NON_POSITIVE_JOBS_TO_DELETE); + return; + } int defaultedDaysOld = daysOld.or(DEFAULT_DAYS_OLD); - // Only generate the detailed response payload if there aren't too many jobs involved. - boolean generatePayload = + boolean verbose = numJobsToDelete.isPresent() && (numJobsToDelete.get() <= DEFAULT_MAX_NUM_JOBS_TO_DELETE); - Optional payloadBuilder = - generatePayload ? Optional.of(new StringBuilder()) : Optional.absent(); - String defaultPayload = "done"; + StringBuilder payload = new StringBuilder(); // Since findEligibleJobsByJobName returns only a certain number of jobs, we must loop through - // until we find enough, requesting deletion as we go. We also stop if we don't find anything, - // or if there are no more jobs to be found (because no cursor is returned). - int numJobsDeletedSoFar = 0; - boolean isFirstTime = true; - Optional cursor = Optional.absent(); + // until we find enough, requesting deletion as we go. + int numJobsProcessed = 0; DateTime cutoffDate = clock.nowUtc().minusDays(defaultedDaysOld); - while ((isFirstTime || cursor.isPresent()) - && (!numJobsToDelete.isPresent() || (numJobsDeletedSoFar < numJobsToDelete.get()))) { - isFirstTime = false; - EligibleJobResults eligibleJobResults = + Optional cursor = Optional.absent(); + do { + EligibleJobResults batch = mapreduceEntityCleanupUtil.findEligibleJobsByJobName( jobName.orNull(), cutoffDate, numJobsToDelete, force.or(false), cursor); - cursor = eligibleJobResults.cursor(); - if (eligibleJobResults.eligibleJobs().isEmpty()) { - logger.infofmt( - "No eligible job with name '%s' older than %s days old.", - jobName.or("(null)"), defaultedDaysOld); - if (generatePayload) { - payloadBuilder.get().append("No eligible job."); + cursor = batch.cursor(); + // Individual batches can come back empty if none of the returned jobs meet the requirements + // or if all jobs have been exhausted. + if (!batch.eligibleJobs().isEmpty()) { + String payloadChunk = requestDeletion(batch.eligibleJobs(), verbose); + if (verbose) { + payload.append(payloadChunk); } - defaultPayload = "No eligible job."; - } else { - String payloadChunk = requestDeletion(eligibleJobResults.eligibleJobs(), generatePayload); - if (generatePayload) { - payloadBuilder.get().append(payloadChunk); - } - numJobsDeletedSoFar += eligibleJobResults.eligibleJobs().size(); + numJobsProcessed += batch.eligibleJobs().size(); } - } + // Stop iterating when all jobs have been exhausted (cursor is absent) or enough have been + // processed. + } while (cursor.isPresent() + && (!numJobsToDelete.isPresent() || (numJobsProcessed < numJobsToDelete.get()))); - logger.infofmt("A total of %s job(s) processed", numJobsDeletedSoFar); - if (generatePayload) { - payloadBuilder - .get() - .append(String.format("A total of %d job(s) processed\n", numJobsDeletedSoFar)); - response.setPayload(payloadBuilder.get().toString()); + if (numJobsProcessed == 0) { + logger.infofmt( + "No eligible jobs found with name '%s' older than %s days old.", + jobName.or("(any)"), defaultedDaysOld); + payload.append("No eligible jobs found"); } else { - response.setPayload(defaultPayload); + logger.infofmt("A total of %s job(s) processed.", numJobsProcessed); + payload.append(String.format("A total of %d job(s) processed", numJobsProcessed)); } + response.setPayload(payload.toString()); } - private String requestDeletion(Set actualJobIds, boolean generatePayload) { + private String requestDeletion(Set actualJobIds, boolean verbose) { Optional payloadChunkBuilder = - generatePayload ? Optional.of(new StringBuilder()) : Optional.absent(); + verbose ? Optional.of(new StringBuilder()) : Optional.absent(); int errorCount = 0; for (String actualJobId : actualJobIds) { Optional error = diff --git a/javatests/google/registry/batch/MapreduceEntityCleanupActionTest.java b/javatests/google/registry/batch/MapreduceEntityCleanupActionTest.java index 6afd1320f..2b6e4a7f8 100644 --- a/javatests/google/registry/batch/MapreduceEntityCleanupActionTest.java +++ b/javatests/google/registry/batch/MapreduceEntityCleanupActionTest.java @@ -15,7 +15,9 @@ package google.registry.batch; import static com.google.appengine.api.datastore.DatastoreServiceFactory.getDatastoreService; +import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; import static com.google.common.truth.Truth.assertThat; +import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import static javax.servlet.http.HttpServletResponse.SC_OK; import static org.joda.time.DateTimeZone.UTC; @@ -45,7 +47,6 @@ import com.google.appengine.tools.pipeline.impl.model.ShardedValue; import com.google.appengine.tools.pipeline.impl.model.Slot; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; -import com.google.common.net.MediaType; import google.registry.testing.ExceptionRule; import google.registry.testing.FakeClock; import google.registry.testing.FakeResponse; @@ -61,8 +62,7 @@ import org.junit.runners.JUnit4; public class MapreduceEntityCleanupActionTest extends MapreduceTestCase { - @Rule - public final ExceptionRule thrown = new ExceptionRule(); + @Rule public final ExceptionRule thrown = new ExceptionRule(); private static final DatastoreService datastore = getDatastoreService(); private static final FetchOptions FETCH_OPTIONS = FetchOptions.Builder.withChunkSize(200); @@ -118,17 +118,6 @@ public class MapreduceEntityCleanupActionTest return pipelineService.startNewPipeline(mapReduceJob, new JobSetting.OnQueue(QUEUE_NAME)); } - /* - private void setJobIdAndJobName(Optional jobId, Optional jobName) { - setJobIdJobNameAndDaysOld(jobId, jobName, Optional.absent()); - } - */ - - private void setAnyJob() { - setJobIdJobNameAndDaysOld( - Optional.absent(), Optional.absent(), Optional.absent()); - } - private void setAnyJobAndDaysOld(int daysOld) { setJobIdJobNameAndDaysOld( Optional.absent(), Optional.absent(), Optional.of(daysOld)); @@ -205,7 +194,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).isEqualTo( jobId + ": deletion requested\n" @@ -240,8 +229,8 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("No eligible job."); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("No eligible jobs found"); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); } @@ -254,9 +243,9 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); - assertThat(response.getPayload()).isEqualTo("No eligible job."); + assertThat(response.getPayload()).isEqualTo("No eligible jobs found"); } @Test @@ -268,49 +257,25 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("done"); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("A total of 1 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(0, 0); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); } - @Test - public void testDeleteZeroJobs_succeeds() throws Exception { - createMapreduce("jobname"); - executeTasksUntilEmpty(QUEUE_NAME, clock); - action = new MapreduceEntityCleanupAction( - Optional.absent(), // jobId - Optional.absent(), // jobName - Optional.of(0), // numJobsToDelete - Optional.absent(), // daysOld - Optional.absent(), // force - mapreduceEntityCleanupUtil, - clock, - DatastoreServiceFactory.getDatastoreService(), - response); - - action.run(); - - assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("A total of 0 job(s) processed\n"); - executeTasksUntilEmpty(QUEUE_NAME, clock); - assertNumMapreducesAndShardedJobs(1, 3); - assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(0); - } - @Test public void testAnyJob_fails() throws Exception { createMapreduce("jobname"); executeTasksUntilEmpty(QUEUE_NAME, clock); - setAnyJob(); + setJobIdJobNameAndDaysOld( + Optional.absent(), Optional.absent(), Optional.absent()); action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("No eligible job."); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("No eligible jobs found"); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); } @@ -323,8 +288,8 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("done"); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("A total of 1 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(0, 0); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); @@ -337,7 +302,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).isEqualTo( "nonexistent: deletion requested\n" + "successfully requested async deletion of 1 job(s); errors received on 0\n"); @@ -356,8 +321,8 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("done"); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("A total of 2 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(0, 0); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); @@ -374,8 +339,8 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("done"); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("A total of 2 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(0, 0); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(2); @@ -401,11 +366,11 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).endsWith( ": deletion requested\n" + "successfully requested async deletion of 1 job(s); errors received on 0\n" - + "A total of 1 job(s) processed\n"); + + "A total of 1 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(1, 3); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); @@ -421,8 +386,8 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("done"); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()).isEqualTo("A total of 1 job(s) processed"); executeTasksUntilEmpty(QUEUE_NAME, clock); assertNumMapreducesAndShardedJobs(1, 3); assertThat(mapreduceEntityCleanupUtil.getNumSearchesPerformed()).isEqualTo(1); @@ -438,7 +403,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).endsWith( ": deletion requested\n" + "successfully requested async deletion of 1 job(s); errors received on 0\n"); @@ -457,7 +422,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).isEqualTo( jobId1 + ": deletion requested\n" @@ -479,7 +444,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response2.getStatus()).isEqualTo(SC_OK); - assertThat(response2.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response2.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response2.getPayload()).isEqualTo( jobId2 + ": deletion requested\n" @@ -498,7 +463,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).endsWith( ": Job is not in FINALIZED or STOPPED state\n" + "successfully requested async deletion of 0 job(s); errors received on 1\n"); @@ -524,7 +489,7 @@ public class MapreduceEntityCleanupActionTest action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).endsWith( ": deletion requested\n" + "successfully requested async deletion of 1 job(s); errors received on 0\n"); @@ -540,8 +505,8 @@ public class MapreduceEntityCleanupActionTest action.run(); - assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).isEqualTo("Do not specify both a job ID and a job name"); assertNumMapreducesAndShardedJobs(0, 0); } @@ -552,8 +517,8 @@ public class MapreduceEntityCleanupActionTest action.run(); - assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()) .isEqualTo("Do not specify both a job ID and a days old threshold"); assertNumMapreducesAndShardedJobs(0, 0); @@ -561,7 +526,6 @@ public class MapreduceEntityCleanupActionTest @Test public void testJobIdAndNumJobs_fails() throws Exception { - clock.setTo(DateTime.now(UTC)); action = new MapreduceEntityCleanupAction( Optional.of("jobid"), Optional.absent(), // jobName @@ -575,10 +539,29 @@ public class MapreduceEntityCleanupActionTest action.run(); - assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()) .isEqualTo("Do not specify both a job ID and a number of jobs to delete"); assertNumMapreducesAndShardedJobs(0, 0); } + + @Test + public void testDeleteZeroJobs_throwsUsageError() throws Exception { + new MapreduceEntityCleanupAction( + Optional.absent(), // jobId + Optional.absent(), // jobName + Optional.of(0), // numJobsToDelete + Optional.absent(), // daysOld + Optional.absent(), // force + mapreduceEntityCleanupUtil, + clock, + DatastoreServiceFactory.getDatastoreService(), + response) + .run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()) + .isEqualTo("Do not specify a non-positive integer for the number of jobs to delete"); + } + }