From 577caabec1cce968df56156d7561a1ffde4b01c4 Mon Sep 17 00:00:00 2001
From: Antonin Delpeuch <antonin@delpeuch.eu>
Date: Thu, 1 Apr 2021 17:10:17 +0200
Subject: [PATCH] Add exponential backoff retries for reconciliation calls.
 (#3770)

* Update test to demonstrate retry mechanism for recon queries.

This was fixed earlier by #3237. Closes #3369.

* Ensure non-zero retry interval in HttpClient

* Restore original bound
---
 main/src/com/google/refine/util/HttpClient.java        | 10 ++++++++--
 .../refine/model/recon/StandardReconConfigTests.java   |  9 ++++-----
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/main/src/com/google/refine/util/HttpClient.java b/main/src/com/google/refine/util/HttpClient.java
index d55955b48..6a75ac1f6 100644
--- a/main/src/com/google/refine/util/HttpClient.java
+++ b/main/src/com/google/refine/util/HttpClient.java
@@ -46,13 +46,19 @@ public class HttpClient {
     private HttpClientBuilder httpClientBuilder;
     private CloseableHttpClient httpClient;
     private int _delay;
+    private int _retryInterval; // delay between original request and first retry, in ms
     
     public HttpClient() {
         this(0);
     }
     
     public HttpClient(int delay) {
+        this(delay, Math.max(delay, 200));
+    }
+    
+    public HttpClient(int delay, int retryInterval) {   
         _delay = delay;
+        _retryInterval = retryInterval;
         // Create a connection manager with a custom socket timeout
         PoolingHttpClientConnectionManager connManager = new PoolingHttpClientConnectionManager();
         final SocketConfig socketConfig = SocketConfig.custom()
@@ -70,7 +76,7 @@ public class HttpClient {
                 .setDefaultRequestConfig(defaultRequestConfig)
                 .setConnectionManager(connManager)
                 // Default Apache HC retry is 1x @1 sec (or the value in Retry-Header)
-                .setRetryStrategy(new ExponentialBackoffRetryStrategy(3, TimeValue.ofMilliseconds(_delay)))
+                .setRetryStrategy(new ExponentialBackoffRetryStrategy(3, TimeValue.ofMilliseconds(_retryInterval)))
 //                .setRedirectStrategy(new LaxRedirectStrategy()) // TODO: No longer needed since default doesn't exclude POST?
 //               .setConnectionBackoffStrategy(ConnectionBackoffStrategy)
                 .addRequestInterceptorFirst(new HttpRequestInterceptor() {
@@ -198,7 +204,7 @@ public class HttpClient {
             // If it's the same as the default, there was no Retry-After, so use binary
             // exponential backoff
             if (interval.compareTo(defaultInterval) == 0) {
-                interval = TimeValue.of(((Double) (Math.pow(2, execCount) * defaultInterval.getDuration())).longValue(),
+                interval = TimeValue.of(((Double) (Math.pow(2, execCount - 1) * defaultInterval.getDuration())).longValue(),
                        defaultInterval.getTimeUnit() );
                 return interval;
             }
diff --git a/main/tests/server/src/com/google/refine/model/recon/StandardReconConfigTests.java b/main/tests/server/src/com/google/refine/model/recon/StandardReconConfigTests.java
index d01a750c9..8ab54ec19 100644
--- a/main/tests/server/src/com/google/refine/model/recon/StandardReconConfigTests.java
+++ b/main/tests/server/src/com/google/refine/model/recon/StandardReconConfigTests.java
@@ -337,9 +337,8 @@ public class StandardReconConfigTests extends RefineTest {
         try (MockWebServer server = new MockWebServer()) {
             server.start();
             HttpUrl url = server.url("/openrefine-wikidata/en/api");
-            // FIXME: Retry doesn't currently work, but should be tested
-//            server.enqueue(new MockResponse().setResponseCode(503)); // service overloaded
-            server.enqueue(new MockResponse().setBody(reconResponse));
+            server.enqueue(new MockResponse().setResponseCode(503)); // service initially overloaded
+            server.enqueue(new MockResponse().setBody(reconResponse)); // service returns successfully
             server.enqueue(new MockResponse());
 
             String configJson = " {\n" +
@@ -366,13 +365,13 @@ public class StandardReconConfigTests extends RefineTest {
             process.startPerforming(pm);
             Assert.assertTrue(process.isRunning());
             try {
-                Thread.sleep(1000); // TODO: timeout will need to increase for retries
+                Thread.sleep(1500);
             } catch (InterruptedException e) {
                 Assert.fail("Test interrupted");
             }
             Assert.assertFalse(process.isRunning());
 
-//          RecordedRequest scratchFirstRquest = server.takeRequest();
+            server.takeRequest(); // ignore the first request which was a 503 error
             RecordedRequest request1 = server.takeRequest();
 
             assertNotNull(request1);