From b7a98451e6af9919de8b1927d8c29d3b0c5ca4a7 Mon Sep 17 00:00:00 2001 From: ctingue Date: Fri, 4 Nov 2016 11:13:59 -0700 Subject: [PATCH] Add retry logic for SheetSynchronizer ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=138211996 --- .../export/sheet/SheetSynchronizer.java | 28 +++++-- .../flows/async/AsyncFlowEnqueuer.java | 10 +-- javatests/google/registry/export/sheet/BUILD | 1 + .../export/sheet/SheetSynchronizerTest.java | 75 ++++++++++++++----- 4 files changed, 87 insertions(+), 27 deletions(-) diff --git a/java/google/registry/export/sheet/SheetSynchronizer.java b/java/google/registry/export/sheet/SheetSynchronizer.java index 627a01590..c49561524 100644 --- a/java/google/registry/export/sheet/SheetSynchronizer.java +++ b/java/google/registry/export/sheet/SheetSynchronizer.java @@ -23,9 +23,11 @@ import com.google.gdata.data.spreadsheet.ListFeed; import com.google.gdata.data.spreadsheet.SpreadsheetEntry; import com.google.gdata.data.spreadsheet.WorksheetEntry; import com.google.gdata.util.ServiceException; +import google.registry.util.Retrier; import java.io.IOException; import java.net.URL; import java.util.List; +import java.util.concurrent.Callable; import javax.inject.Inject; /** Generic data synchronization utility for Google Spreadsheets. */ @@ -36,6 +38,7 @@ class SheetSynchronizer { @Inject SpreadsheetService spreadsheetService; @Inject SheetSynchronizer() {} + @Inject Retrier retrier; /** * Replace the contents of a Google Spreadsheet with {@code data}. @@ -73,11 +76,12 @@ class SheetSynchronizer { WorksheetEntry worksheet = spreadsheet.getWorksheets().get(0); worksheet.setRowCount(data.size() + 1); // account for header row worksheet = worksheet.update(); - ListFeed listFeed = spreadsheetService.getFeed(worksheet.getListFeedUrl(), ListFeed.class); + final ListFeed listFeed = + spreadsheetService.getFeed(worksheet.getListFeedUrl(), ListFeed.class); List entries = listFeed.getEntries(); int commonSize = Math.min(entries.size(), data.size()); for (int i = 0; i < commonSize; i++) { - ListEntry entry = entries.get(i); + final ListEntry entry = entries.get(i); CustomElementCollection elements = entry.getCustomElements(); boolean mutated = false; for (ImmutableMap.Entry cell : data.get(i).entrySet()) { @@ -87,17 +91,31 @@ class SheetSynchronizer { } } if (mutated) { - entry.update(); + // Wrap in a retrier to deal with transient HTTP failures, which are IOExceptions wrapped + // in RuntimeExceptions. + retrier.callWithRetry(new Callable() { + @Override + public Void call() throws Exception { + entry.update(); + return null; + }}, RuntimeException.class); } } if (data.size() > entries.size()) { for (int i = entries.size(); i < data.size(); i++) { - ListEntry entry = listFeed.createEntry(); + final ListEntry entry = listFeed.createEntry(); CustomElementCollection elements = entry.getCustomElements(); for (ImmutableMap.Entry cell : data.get(i).entrySet()) { elements.setValueLocal(cell.getKey(), cell.getValue()); } - listFeed.insert(entry); + // Wrap in a retrier to deal with transient HTTP failures, which are IOExceptions wrapped + // in RuntimeExceptions. + retrier.callWithRetry(new Callable() { + @Override + public Void call() throws Exception { + listFeed.insert(entry); + return null; + }}, RuntimeException.class); } } } diff --git a/java/google/registry/flows/async/AsyncFlowEnqueuer.java b/java/google/registry/flows/async/AsyncFlowEnqueuer.java index de9f50aff..5071f7555 100644 --- a/java/google/registry/flows/async/AsyncFlowEnqueuer.java +++ b/java/google/registry/flows/async/AsyncFlowEnqueuer.java @@ -78,10 +78,10 @@ public final class AsyncFlowEnqueuer { */ private void addTaskToQueueWithRetry(final Queue queue, final TaskOptions task) { retrier.callWithRetry(new Callable() { - @Override - public Void call() throws Exception { - queue.add(task); - return null; - }}, TransientFailureException.class); + @Override + public Void call() throws Exception { + queue.add(task); + return null; + }}, TransientFailureException.class); } } diff --git a/javatests/google/registry/export/sheet/BUILD b/javatests/google/registry/export/sheet/BUILD index a0a67d871..eb9056d5c 100644 --- a/javatests/google/registry/export/sheet/BUILD +++ b/javatests/google/registry/export/sheet/BUILD @@ -23,6 +23,7 @@ java_library( "//java/google/registry/config", "//java/google/registry/export/sheet", "//java/google/registry/model", + "//java/google/registry/util", "//javatests/google/registry/testing", ], ) diff --git a/javatests/google/registry/export/sheet/SheetSynchronizerTest.java b/javatests/google/registry/export/sheet/SheetSynchronizerTest.java index 58c91d370..b5f747c51 100644 --- a/javatests/google/registry/export/sheet/SheetSynchronizerTest.java +++ b/javatests/google/registry/export/sheet/SheetSynchronizerTest.java @@ -18,6 +18,7 @@ import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -30,7 +31,11 @@ import com.google.gdata.data.spreadsheet.ListEntry; import com.google.gdata.data.spreadsheet.ListFeed; import com.google.gdata.data.spreadsheet.SpreadsheetEntry; import com.google.gdata.data.spreadsheet.WorksheetEntry; +import google.registry.testing.FakeClock; +import google.registry.testing.FakeSleeper; +import google.registry.util.Retrier; import java.net.URL; +import org.joda.time.DateTime; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -41,32 +46,34 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class SheetSynchronizerTest { + private static final int MAX_RETRIES = 3; + private final SpreadsheetService spreadsheetService = mock(SpreadsheetService.class); private final SpreadsheetEntry spreadsheet = mock(SpreadsheetEntry.class); private final WorksheetEntry worksheet = mock(WorksheetEntry.class); private final ListFeed listFeed = mock(ListFeed.class); private final SheetSynchronizer sheetSynchronizer = new SheetSynchronizer(); + private final FakeSleeper sleeper = + new FakeSleeper(new FakeClock(DateTime.parse("2000-01-01TZ"))); @Before public void before() throws Exception { sheetSynchronizer.spreadsheetService = spreadsheetService; + sheetSynchronizer.retrier = new Retrier(sleeper, MAX_RETRIES); when(spreadsheetService.getEntry(any(URL.class), eq(SpreadsheetEntry.class))) .thenReturn(spreadsheet); - when(spreadsheet.getWorksheets()) - .thenReturn(ImmutableList.of(worksheet)); - when(worksheet.getListFeedUrl()) - .thenReturn(new URL("http://example.com/spreadsheet")); - when(spreadsheetService.getFeed(any(URL.class), eq(ListFeed.class))) - .thenReturn(listFeed); - when(worksheet.update()) - .thenReturn(worksheet); + when(spreadsheet.getWorksheets()).thenReturn(ImmutableList.of(worksheet)); + when(worksheet.getListFeedUrl()).thenReturn(new URL("http://example.com/spreadsheet")); + when(spreadsheetService.getFeed(any(URL.class), eq(ListFeed.class))).thenReturn(listFeed); + when(worksheet.update()).thenReturn(worksheet); } @After public void after() throws Exception { - verify(spreadsheetService).getEntry( - new URL("https://spreadsheets.google.com/feeds/spreadsheets/foobar"), - SpreadsheetEntry.class); + verify(spreadsheetService) + .getEntry( + new URL("https://spreadsheets.google.com/feeds/spreadsheets/foobar"), + SpreadsheetEntry.class); verify(spreadsheet).getWorksheets(); verify(worksheet).getListFeedUrl(); verify(spreadsheetService).getFeed(new URL("http://example.com/spreadsheet"), ListFeed.class); @@ -86,8 +93,7 @@ public class SheetSynchronizerTest { public void testSynchronize_bothContainSameRow_doNothing() throws Exception { ListEntry entry = makeListEntry(ImmutableMap.of("key", "value")); when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry)); - sheetSynchronizer.synchronize("foobar", ImmutableList.of( - ImmutableMap.of("key", "value"))); + sheetSynchronizer.synchronize("foobar", ImmutableList.of(ImmutableMap.of("key", "value"))); verify(worksheet).setRowCount(2); verify(worksheet).update(); verify(entry, atLeastOnce()).getCustomElements(); @@ -98,8 +104,7 @@ public class SheetSynchronizerTest { public void testSynchronize_cellIsDifferent_updateRow() throws Exception { ListEntry entry = makeListEntry(ImmutableMap.of("key", "value")); when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry)); - sheetSynchronizer.synchronize("foobar", ImmutableList.of( - ImmutableMap.of("key", "new value"))); + sheetSynchronizer.synchronize("foobar", ImmutableList.of(ImmutableMap.of("key", "new value"))); verify(entry.getCustomElements()).setValueLocal("key", "new value"); verify(entry).update(); verify(worksheet).setRowCount(2); @@ -108,13 +113,29 @@ public class SheetSynchronizerTest { verifyNoMoreInteractions(entry); } + @Test + public void testSynchronize_cellIsDifferent_updateRow_retriesOnException() throws Exception { + ListEntry entry = makeListEntry(ImmutableMap.of("key", "value")); + when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry)); + when(entry.update()) + .thenThrow(new RuntimeException()) + .thenThrow(new RuntimeException()) + .thenReturn(entry); + sheetSynchronizer.synchronize("foobar", ImmutableList.of(ImmutableMap.of("key", "new value"))); + verify(entry.getCustomElements()).setValueLocal("key", "new value"); + verify(entry, times(3)).update(); + verify(worksheet).setRowCount(2); + verify(worksheet).update(); + verify(entry, atLeastOnce()).getCustomElements(); + verifyNoMoreInteractions(entry); + } + @Test public void testSynchronize_spreadsheetMissingRow_insertRow() throws Exception { ListEntry entry = makeListEntry(ImmutableMap.of()); when(listFeed.getEntries()).thenReturn(ImmutableList.of()); when(listFeed.createEntry()).thenReturn(entry); - sheetSynchronizer.synchronize("foobar", ImmutableList.of( - ImmutableMap.of("key", "value"))); + sheetSynchronizer.synchronize("foobar", ImmutableList.of(ImmutableMap.of("key", "value"))); verify(entry.getCustomElements()).setValueLocal("key", "value"); verify(listFeed).insert(entry); verify(worksheet).setRowCount(2); @@ -124,6 +145,26 @@ public class SheetSynchronizerTest { verifyNoMoreInteractions(entry); } + @Test + public void testSynchronize_spreadsheetMissingRow_insertRow_retriesOnException() + throws Exception { + ListEntry entry = makeListEntry(ImmutableMap.of()); + when(listFeed.getEntries()).thenReturn(ImmutableList.of()); + when(listFeed.createEntry()).thenReturn(entry); + when(listFeed.insert(entry)) + .thenThrow(new RuntimeException()) + .thenThrow(new RuntimeException()) + .thenReturn(entry); + sheetSynchronizer.synchronize("foobar", ImmutableList.of(ImmutableMap.of("key", "value"))); + verify(entry.getCustomElements()).setValueLocal("key", "value"); + verify(listFeed, times(3)).insert(entry); + verify(worksheet).setRowCount(2); + verify(worksheet).update(); + verify(listFeed).createEntry(); + verify(entry, atLeastOnce()).getCustomElements(); + verifyNoMoreInteractions(entry); + } + @Test public void testSynchronize_spreadsheetRowNoLongerInData_deleteRow() throws Exception { ListEntry entry = makeListEntry(ImmutableMap.of("key", "value"));