Refactor main loop of MapreduceEntityCleanupAction

This also tightens up some error-checking conditions.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149552065
This commit is contained in:
mcilwain 2017-03-08 10:11:04 -08:00 committed by Ben McIlwain
parent 9c33245200
commit d2ca4b7234
2 changed files with 100 additions and 117 deletions

View file

@ -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<StringBuilder> payloadBuilder =
generatePayload ? Optional.of(new StringBuilder()) : Optional.<StringBuilder>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<String> cursor = Optional.<String>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<String> 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<String> actualJobIds, boolean generatePayload) {
private String requestDeletion(Set<String> actualJobIds, boolean verbose) {
Optional<StringBuilder> payloadChunkBuilder =
generatePayload ? Optional.of(new StringBuilder()) : Optional.<StringBuilder>absent();
verbose ? Optional.of(new StringBuilder()) : Optional.<StringBuilder>absent();
int errorCount = 0;
for (String actualJobId : actualJobIds) {
Optional<String> error =

View file

@ -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<MapreduceEntityCleanupAction> {
@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<String> jobId, Optional<String> jobName) {
setJobIdJobNameAndDaysOld(jobId, jobName, Optional.<Integer>absent());
}
*/
private void setAnyJob() {
setJobIdJobNameAndDaysOld(
Optional.<String>absent(), Optional.<String>absent(), Optional.<Integer>absent());
}
private void setAnyJobAndDaysOld(int daysOld) {
setJobIdJobNameAndDaysOld(
Optional.<String>absent(), Optional.<String>absent(), Optional.<Integer>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.<String>absent(), // jobId
Optional.<String>absent(), // jobName
Optional.<Integer>of(0), // numJobsToDelete
Optional.<Integer>absent(), // daysOld
Optional.<Boolean>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.<String>absent(), Optional.<String>absent(), Optional.<Integer>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.<String>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.<String>absent(), // jobId
Optional.<String>absent(), // jobName
Optional.<Integer>of(0), // numJobsToDelete
Optional.<Integer>absent(), // daysOld
Optional.<Boolean>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");
}
}