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(); + } }