From 8aeedd3bd564a6cc197ca4ac7a1b2dba8e78ce2d Mon Sep 17 00:00:00 2001 From: Jae Kim <45045038+jaeopt@users.noreply.github.com> Date: Thu, 27 Apr 2023 16:38:32 -0700 Subject: [PATCH] [FSSDK-9103] feat(ATS): fix retries for ODP segment and event API calls. (#454) Fix retries for ODP api access - no retry on fetchQualifiedSegments failure - max 3 retries on ODP event dispatch --- .../ab/android/odp/ODPEventClientTest.kt | 9 ++++----- .../ab/android/odp/ODPSegmentClientTest.kt | 15 +++++++-------- .../optimizely/ab/android/odp/ODPEventClient.kt | 6 +++++- .../optimizely/ab/android/odp/ODPSegmentClient.kt | 15 +++++++++------ .../optimizely/ab/android/shared/ClientTest.java | 9 +++++++++ .../com/optimizely/ab/android/shared/Client.java | 3 +++ 6 files changed, 37 insertions(+), 20 deletions(-) diff --git a/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPEventClientTest.kt b/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPEventClientTest.kt index 21fa8589..67501b82 100644 --- a/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPEventClientTest.kt +++ b/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPEventClientTest.kt @@ -25,7 +25,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.Matchers.any -import org.mockito.Matchers.anyInt import org.mockito.Matchers.contains import org.mockito.Matchers.eq import org.mockito.Mockito.`when` @@ -76,7 +75,7 @@ class ODPEventClientTest { eventClient.dispatch(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(2), eq(3)) val received = captor.value.execute() as Boolean assertFalse(received) @@ -91,7 +90,7 @@ class ODPEventClientTest { eventClient.dispatch(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(2), eq(3)) val received = captor.value.execute() as Boolean assertFalse(received) @@ -107,10 +106,10 @@ class ODPEventClientTest { apiEndpoint = "invalid-url" eventClient.dispatch(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(2), eq(3)) val received = captor.value.execute() as Boolean assertFalse(received) - verify(logger).error(contains("Error making request"), any()) + verify(logger).error(contains("Error making ODP event request"), any()) } } diff --git a/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPSegmentClientTest.kt b/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPSegmentClientTest.kt index b7e0f37e..c7a2a82e 100644 --- a/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPSegmentClientTest.kt +++ b/odp/src/androidTest/java/com/optimizely/ab/android/odp/ODPSegmentClientTest.kt @@ -24,7 +24,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.Matchers.any -import org.mockito.Matchers.anyInt import org.mockito.Matchers.contains import org.mockito.Matchers.eq import org.mockito.Mockito.`when` @@ -59,7 +58,7 @@ class ODPSegmentClientTest { segmentClient.fetchQualifiedSegments(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), eq(2), eq(2)) + verify(client).execute(captor.capture(), eq(0), eq(0)) val received = captor.value.execute() assert(received == response) @@ -75,11 +74,11 @@ class ODPSegmentClientTest { segmentClient.fetchQualifiedSegments(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(0), eq(0)) val received = captor.value.execute() assertNull(received) - verify(logger).error("Unexpected response from event endpoint, status: 400") + verify(logger).error("Unexpected response from ODP segment endpoint, status: 400") verify(urlConnection).disconnect() } @@ -89,11 +88,11 @@ class ODPSegmentClientTest { segmentClient.fetchQualifiedSegments(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(0), eq(0)) val received = captor.value.execute() assertNull(received) - verify(logger).error("Unexpected response from event endpoint, status: 500") + verify(logger).error("Unexpected response from ODP segment endpoint, status: 500") verify(urlConnection).disconnect() } @@ -104,10 +103,10 @@ class ODPSegmentClientTest { apiEndpoint = "invalid-url" segmentClient.fetchQualifiedSegments(apiKey, apiEndpoint, payload) - verify(client).execute(captor.capture(), anyInt(), anyInt()) + verify(client).execute(captor.capture(), eq(0), eq(0)) val received = captor.value.execute() assertNull(received) - verify(logger).error(contains("Error making request"), any()) + verify(logger).error(contains("Error making ODP segment request"), any()) } } diff --git a/odp/src/main/java/com/optimizely/ab/android/odp/ODPEventClient.kt b/odp/src/main/java/com/optimizely/ab/android/odp/ODPEventClient.kt index 1370bb76..a2d072d9 100644 --- a/odp/src/main/java/com/optimizely/ab/android/odp/ODPEventClient.kt +++ b/odp/src/main/java/com/optimizely/ab/android/odp/ODPEventClient.kt @@ -69,7 +69,7 @@ open class ODPEventClient(private val client: Client, private val logger: Logger return@Request false } } catch (e: Exception) { - logger.error("Error making request", e) + logger.error("Error making ODP event request", e) return@Request false } finally { if (urlConnection != null) { @@ -90,6 +90,10 @@ open class ODPEventClient(private val client: Client, private val logger: Logger var CONNECTION_TIMEOUT = 10 * 1000 var READ_TIMEOUT = 60 * 1000 + // OdpEventManager (java-sdk core) is supposed to handle retries on failures. + // android-sdk returns success immediately for sendOdpEvent() from OdpEventManager and schedules it via WorkManager. + // so retries on failure are supported here in OdpEventClient for android-sdk. + // the numerical base for the exponential backoff const val REQUEST_BACKOFF_TIMEOUT = 2 // power the number of retries diff --git a/odp/src/main/java/com/optimizely/ab/android/odp/ODPSegmentClient.kt b/odp/src/main/java/com/optimizely/ab/android/odp/ODPSegmentClient.kt index abab37cd..beed6aa0 100644 --- a/odp/src/main/java/com/optimizely/ab/android/odp/ODPSegmentClient.kt +++ b/odp/src/main/java/com/optimizely/ab/android/odp/ODPSegmentClient.kt @@ -57,14 +57,14 @@ open class ODPSegmentClient(private val client: Client, private val logger: Logg val status = urlConnection.responseCode if (status in 200..399) { val json = client.readStream(urlConnection) - logger.debug("Successfully fetched segments: {}", json) + logger.debug("Successfully fetched ODP segments: {}", json) return@Request json } else { - logger.error("Unexpected response from event endpoint, status: $status") + logger.error("Unexpected response from ODP segment endpoint, status: $status") return@Request null } } catch (e: Exception) { - logger.error("Error making request", e) + logger.error("Error making ODP segment request", e) return@Request null } finally { if (urlConnection != null) { @@ -92,9 +92,12 @@ open class ODPSegmentClient(private val client: Client, private val logger: Logg var CONNECTION_TIMEOUT = 10 * 1000 var READ_TIMEOUT = 60 * 1000 + // No retries on fetchQualifiedSegments() errors. + // We want to return failure immediately to callers. + // the numerical base for the exponential backoff - const val REQUEST_BACKOFF_TIMEOUT = 2 - // power the number of retries (2 = retry once) - const val REQUEST_RETRIES_POWER = 2 + const val REQUEST_BACKOFF_TIMEOUT = 0 + // power the number of retries + const val REQUEST_RETRIES_POWER = 0 } } diff --git a/shared/src/androidTest/java/com/optimizely/ab/android/shared/ClientTest.java b/shared/src/androidTest/java/com/optimizely/ab/android/shared/ClientTest.java index e1b3e29b..8bdd6038 100644 --- a/shared/src/androidTest/java/com/optimizely/ab/android/shared/ClientTest.java +++ b/shared/src/androidTest/java/com/optimizely/ab/android/shared/ClientTest.java @@ -144,4 +144,13 @@ public void testExpBackoffFailure() { assertTrue(timeouts.contains(8)); assertTrue(timeouts.contains(16)); } + + @Test + public void testExpBackoffFailure_noRetriesWhenBackoffSetToZero() { + Client.Request request = mock(Client.Request.class); + when(request.execute()).thenReturn(null); + assertNull(client.execute(request, 0, 0)); + verify(logger, never()).info(eq("Request failed, waiting {} seconds to try again"), any(Integer.class)); + } + } diff --git a/shared/src/main/java/com/optimizely/ab/android/shared/Client.java b/shared/src/main/java/com/optimizely/ab/android/shared/Client.java index e8d6c436..f2338eac 100644 --- a/shared/src/main/java/com/optimizely/ab/android/shared/Client.java +++ b/shared/src/main/java/com/optimizely/ab/android/shared/Client.java @@ -162,6 +162,9 @@ public T execute(Request request, int timeout, int power) { } if (response == null || response == Boolean.FALSE) { + // retry is disabled when timeout set to 0 + if (timeout == 0) break; + try { logger.info("Request failed, waiting {} seconds to try again", timeout); Thread.sleep(TimeUnit.MILLISECONDS.convert(timeout, TimeUnit.SECONDS));