From 2620097599594807aa29d2028fe3d303871d8a55 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Tue, 6 Dec 2016 12:21:08 -0800 Subject: [PATCH] Retry attempted syncs to Google Groups that fail I also moved to a non-concurrent modification syncing model. It was adding more complexity than was justified just to have two requests going simultaneously instead of one. The API doesn't reliably allow much more than that anyway. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=141210192 --- .../export/SyncGroupMembersAction.java | 67 ++++++++----------- .../export/SyncGroupMembersActionTest.java | 28 +++++++- 2 files changed, 56 insertions(+), 39 deletions(-) diff --git a/java/google/registry/export/SyncGroupMembersAction.java b/java/google/registry/export/SyncGroupMembersAction.java index a93affc64..dac583cea 100644 --- a/java/google/registry/export/SyncGroupMembersAction.java +++ b/java/google/registry/export/SyncGroupMembersAction.java @@ -26,6 +26,7 @@ import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.googlecode.objectify.VoidWork; import google.registry.config.ConfigModule.Config; @@ -35,12 +36,14 @@ import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarContact; import google.registry.request.Action; import google.registry.request.Response; -import google.registry.util.Concurrent; import google.registry.util.FormattingLogger; +import google.registry.util.Retrier; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.concurrent.Callable; import javax.annotation.Nullable; import javax.inject.Inject; @@ -52,14 +55,6 @@ import javax.inject.Inject; @Action(path = "/_dr/task/syncGroupMembers", method = POST) public final class SyncGroupMembersAction implements Runnable { - /** - * The number of threads to run simultaneously (one per registrar) while processing group syncs. - * This number is purposefully low because App Engine will complain about a large number of - * requests per second, so it's better to spread the work out (as we are only running this servlet - * once per hour anyway). - */ - private static final int NUM_WORK_THREADS = 2; - private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); @@ -87,8 +82,9 @@ public final class SyncGroupMembersAction implements Runnable { } @Inject GroupsConnection groupsConnection; - @Inject Response response; @Inject @Config("publicDomainName") String publicDomainName; + @Inject Response response; + @Inject Retrier retrier; @Inject SyncGroupMembersAction() {} private void sendResponse(Result result, @Nullable List causes) { @@ -134,23 +130,24 @@ public final class SyncGroupMembersAction implements Runnable { return; } - // Run multiple threads to communicate with Google Groups simultaneously. - ImmutableList> results = Concurrent.transform( - dirtyRegistrars, - NUM_WORK_THREADS, - new Function>() { + ImmutableMap.Builder> resultsBuilder = + new ImmutableMap.Builder<>(); + for (final Registrar registrar : dirtyRegistrars) { + try { + retrier.callWithRetry(new Callable() { @Override - public Optional apply(final Registrar registrar) { - try { - syncRegistrarContacts(registrar); - return Optional. absent(); - } catch (Throwable e) { - logger.severe(e, e.getMessage()); - return Optional.of(e); - } - }}); + public Void call() throws Exception { + syncRegistrarContacts(registrar); + return null; + }}, RuntimeException.class); + resultsBuilder.put(registrar, Optional.absent()); + } catch (Throwable e) { + logger.severe(e, e.getMessage()); + resultsBuilder.put(registrar, Optional.of(e)); + } + } - List errors = getErrorsAndUpdateFlagsForSuccesses(dirtyRegistrars, results); + List errors = getErrorsAndUpdateFlagsForSuccesses(resultsBuilder.build()); // If there were no errors, return success; otherwise return a failed status and log the errors. if (errors.isEmpty()) { sendResponse(Result.OK, null); @@ -163,18 +160,15 @@ public final class SyncGroupMembersAction implements Runnable { * Parses the results from Google Groups for each registrar, setting the dirty flag to false in * Datastore for the calls that succeeded and accumulating the errors for the calls that failed. */ - private List getErrorsAndUpdateFlagsForSuccesses( - List registrars, - List> results) { + private static List getErrorsAndUpdateFlagsForSuccesses( + ImmutableMap> results) { final ImmutableList.Builder registrarsToSave = new ImmutableList.Builder<>(); List errors = new ArrayList<>(); - for (int i = 0; i < results.size(); i++) { - Optional opt = results.get(i); - if (opt.isPresent()) { - errors.add(opt.get()); + for (Map.Entry> result : results.entrySet()) { + if (result.getValue().isPresent()) { + errors.add(result.getValue().get()); } else { - registrarsToSave.add( - registrars.get(i).asBuilder().setContactsRequireSyncing(false).build()); + registrarsToSave.add(result.getKey().asBuilder().setContactsRequireSyncing(false).build()); } } ofy().transactNew(new VoidWork() { @@ -222,10 +216,7 @@ public final class SyncGroupMembersAction implements Runnable { totalAdded, totalRemoved); } catch (IOException e) { - // Bail out of the current sync job if an error occurs. This is OK because (a) errors usually - // indicate that retrying won't succeed at all, or at least not immediately, and (b) the sync - // job will run within an hour anyway and effectively resume where it left off if this was a - // transient error. + // Package up exception and re-throw with attached additional relevant info. String msg = String.format("Couldn't sync contacts for registrar %s to group %s", registrar.getClientId(), groupKey); throw new RuntimeException(msg, e); diff --git a/javatests/google/registry/export/SyncGroupMembersActionTest.java b/javatests/google/registry/export/SyncGroupMembersActionTest.java index c04519774..a292a920d 100644 --- a/javatests/google/registry/export/SyncGroupMembersActionTest.java +++ b/javatests/google/registry/export/SyncGroupMembersActionTest.java @@ -23,6 +23,10 @@ import static google.registry.model.registrar.RegistrarContact.Type.TECH; import static google.registry.testing.DatastoreHelper.persistResource; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_OK; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -36,7 +40,10 @@ import google.registry.model.registrar.RegistrarContact; import google.registry.request.Response; import google.registry.testing.AppEngineRule; import google.registry.testing.ExceptionRule; +import google.registry.testing.FakeClock; +import google.registry.testing.FakeSleeper; import google.registry.testing.InjectRule; +import google.registry.util.Retrier; import java.io.IOException; import org.junit.Rule; import org.junit.Test; @@ -73,8 +80,9 @@ public class SyncGroupMembersActionTest { private void runAction() { SyncGroupMembersAction action = new SyncGroupMembersAction(); action.groupsConnection = connection; - action.response = response; action.publicDomainName = "domain-registry.example"; + action.response = response; + action.retrier = new Retrier(new FakeSleeper(new FakeClock()), 3); action.run(); } @@ -214,9 +222,27 @@ public class SyncGroupMembersActionTest { "newregistrar-primary-contacts@domain-registry.example", "janedoe@theregistrar.com", Role.MEMBER); + verify(connection, times(3)) + .getMembersOfGroup("theregistrar-primary-contacts@domain-registry.example"); verify(response).setStatus(SC_INTERNAL_SERVER_ERROR); verify(response).setPayload("FAILED Error occurred while updating registrar contacts.\n"); assertThat(Registrar.loadByClientId("NewRegistrar").getContactsRequireSyncing()).isFalse(); assertThat(Registrar.loadByClientId("TheRegistrar").getContactsRequireSyncing()).isTrue(); } + + @Test + public void test_doPost_retriesOnTransientException() throws Exception { + doThrow(IOException.class) + .doNothing() + .when(connection) + .addMemberToGroup(anyString(), anyString(), any(Role.class)); + runAction(); + verify(connection, times(2)).addMemberToGroup( + "newregistrar-primary-contacts@domain-registry.example", + "janedoe@theregistrar.com", + Role.MEMBER); + verify(response).setStatus(SC_OK); + verify(response).setPayload("OK Group memberships successfully updated.\n"); + assertThat(Registrar.loadByClientId("NewRegistrar").getContactsRequireSyncing()).isFalse(); + } }