From 8747cb7cb6ea04e6daba8e39ec4447b83a5b5c16 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 1 May 2024 13:41:15 -0700 Subject: [PATCH 1/6] javalin testing --- core-httpclient-impl/README.md | 28 +++--- core-httpclient-impl/build.gradle | 5 + .../com/optimizely/ab/HttpClientUtils.java | 4 +- .../optimizely/ab/OptimizelyHttpClient.java | 21 ++++- .../ab/event/AsyncEventHandler.java | 21 +++-- .../ab/OptimizelyHttpClientTest.java | 94 +++++++++++++++++-- .../ab/event/AsyncEventHandlerTest.java | 12 +++ 7 files changed, 150 insertions(+), 35 deletions(-) diff --git a/core-httpclient-impl/README.md b/core-httpclient-impl/README.md index 8e70b2ddb..55eac460b 100644 --- a/core-httpclient-impl/README.md +++ b/core-httpclient-impl/README.md @@ -106,24 +106,24 @@ The number of workers determines the number of threads the thread pool uses. ### Builder Methods The following builder methods can be used to custom configure the `AsyncEventHandler`. -|Method Name|Default Value|Description| -|---|---|---| -|`withQueueCapacity(int)`|10000|Queue size for pending logEvents| -|`withNumWorkers(int)`|2|Number of worker threads| -|`withMaxTotalConnections(int)`|200|Maximum number of connections| -|`withMaxPerRoute(int)`|20|Maximum number of connections per route| -|`withValidateAfterInactivity(int)`|5000|Time to maintain idol connections (in milliseconds)| +|Method Name|Default Value| Description | +|---|---|----------------------------------------------------| +|`withQueueCapacity(int)`|10000| Queue size for pending logEvents | +|`withNumWorkers(int)`|2| Number of worker threads | +|`withMaxTotalConnections(int)`|200| Maximum number of connections | +|`withMaxPerRoute(int)`|20| Maximum number of connections per route | +|`withValidateAfterInactivity(int)`|5000| Time to maintain idle connections (in milliseconds) | ### Advanced configuration The following properties can be set to override the default configuration. -|Property Name|Default Value|Description| -|---|---|---| -|**async.event.handler.queue.capacity**|10000|Queue size for pending logEvents| -|**async.event.handler.num.workers**|2|Number of worker threads| -|**async.event.handler.max.connections**|200|Maximum number of connections| -|**async.event.handler.event.max.per.route**|20|Maximum number of connections per route| -|**async.event.handler.validate.after**|5000|Time to maintain idol connections (in milliseconds)| +|Property Name|Default Value| Description | +|---|---|----------------------------------------------------| +|**async.event.handler.queue.capacity**|10000| Queue size for pending logEvents | +|**async.event.handler.num.workers**|2| Number of worker threads | +|**async.event.handler.max.connections**|200| Maximum number of connections | +|**async.event.handler.event.max.per.route**|20| Maximum number of connections per route | +|**async.event.handler.validate.after**|5000| Time to maintain idle connections (in milliseconds) | ## HttpProjectConfigManager diff --git a/core-httpclient-impl/build.gradle b/core-httpclient-impl/build.gradle index b43c70269..b44733ff4 100644 --- a/core-httpclient-impl/build.gradle +++ b/core-httpclient-impl/build.gradle @@ -4,6 +4,11 @@ dependencies { compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion compile group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion + + testImplementation 'io.javalin:javalin:3.13.10' + testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2' + testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.2' + testImplementation 'io.rest-assured:rest-assured:4.4.0' } task exhaustiveTest { diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java index dc786c4f6..88cc775d6 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java @@ -26,7 +26,9 @@ public final class HttpClientUtils { public static final int CONNECTION_TIMEOUT_MS = 10000; public static final int CONNECTION_REQUEST_TIMEOUT_MS = 5000; public static final int SOCKET_TIMEOUT_MS = 10000; - + public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 5000; + public static final int DEFAULT_MAX_CONNECTIONS = 200; + public static final int DEFAULT_MAX_PER_ROUTE = 20; private static RequestConfig requestConfigWithTimeout; private HttpClientUtils() { diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java index 363bce59c..0e026f3e4 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java @@ -17,11 +17,15 @@ package com.optimizely.ab; import com.optimizely.ab.annotations.VisibleForTesting; +import com.optimizely.ab.HttpClientUtils; + import org.apache.http.client.HttpClient; +import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.impl.client.HttpClients; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; @@ -73,16 +77,18 @@ public static class Builder { // The following static values are public so that they can be tweaked if necessary. // These are the recommended settings for http protocol. https://hc.apache.org/httpcomponents-client-ga/tutorial/html/connmgmt.html // The maximum number of connections allowed across all routes. - private int maxTotalConnections = 200; + int maxTotalConnections = HttpClientUtils.DEFAULT_MAX_CONNECTIONS; // The maximum number of connections allowed for a route - private int maxPerRoute = 20; + int maxPerRoute = HttpClientUtils.DEFAULT_MAX_PER_ROUTE; // Defines period of inactivity in milliseconds after which persistent connections must be re-validated prior to being leased to the consumer. - private int validateAfterInactivity = 5000; + int validateAfterInactivity = HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY; // force-close the connection after this idle time (with 0, eviction is disabled by default) long evictConnectionIdleTimePeriod = 0; + HttpRequestRetryHandler customRetryHandler = null; TimeUnit evictConnectionIdleTimeUnit = TimeUnit.MILLISECONDS; private int timeoutMillis = HttpClientUtils.CONNECTION_TIMEOUT_MS; + private Builder() { } @@ -107,6 +113,12 @@ public Builder withEvictIdleConnections(long maxIdleTime, TimeUnit maxIdleTimeUn this.evictConnectionIdleTimeUnit = maxIdleTimeUnit; return this; } + + // customize retryHandler (DefaultHttpRequestRetryHandler will be used by default) + public Builder withRetryHandler(HttpRequestRetryHandler retryHandler) { + this.customRetryHandler = retryHandler; + return this; + } public Builder setTimeoutMillis(int timeoutMillis) { this.timeoutMillis = timeoutMillis; @@ -124,6 +136,9 @@ public OptimizelyHttpClient build() { .setConnectionManager(poolingHttpClientConnectionManager) .disableCookieManagement() .useSystemProperties(); + if (customRetryHandler != null) { + builder.setRetryHandler(customRetryHandler); + } logger.debug("Creating HttpClient with timeout: " + timeoutMillis); diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java index 434b29103..7b550b2c5 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java @@ -16,6 +16,7 @@ */ package com.optimizely.ab.event; +import com.optimizely.ab.HttpClientUtils; import com.optimizely.ab.NamedThreadFactory; import com.optimizely.ab.OptimizelyHttpClient; import com.optimizely.ab.annotations.VisibleForTesting; @@ -31,6 +32,7 @@ import org.apache.http.client.methods.HttpRequestBase; import org.apache.http.client.utils.URIBuilder; import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,7 +47,6 @@ import java.util.concurrent.TimeUnit; import javax.annotation.CheckForNull; -import javax.annotation.Nullable; /** * {@link EventHandler} implementation that queues events and has a separate pool of threads responsible @@ -61,9 +62,7 @@ public class AsyncEventHandler implements EventHandler, AutoCloseable { public static final int DEFAULT_QUEUE_CAPACITY = 10000; public static final int DEFAULT_NUM_WORKERS = 2; - public static final int DEFAULT_MAX_CONNECTIONS = 200; - public static final int DEFAULT_MAX_PER_ROUTE = 20; - public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 5000; + private static final Logger logger = LoggerFactory.getLogger(AsyncEventHandler.class); private static final ProjectConfigResponseHandler EVENT_RESPONSE_HANDLER = new ProjectConfigResponseHandler(); @@ -135,15 +134,17 @@ public AsyncEventHandler(int queueCapacity, if (httpClient != null) { this.httpClient = httpClient; } else { - maxConnections = validateInput("maxConnections", maxConnections, DEFAULT_MAX_CONNECTIONS); - connectionsPerRoute = validateInput("connectionsPerRoute", connectionsPerRoute, DEFAULT_MAX_PER_ROUTE); - validateAfter = validateInput("validateAfter", validateAfter, DEFAULT_VALIDATE_AFTER_INACTIVITY); + maxConnections = validateInput("maxConnections", maxConnections, HttpClientUtils.DEFAULT_MAX_CONNECTIONS); + connectionsPerRoute = validateInput("connectionsPerRoute", connectionsPerRoute, HttpClientUtils.DEFAULT_MAX_PER_ROUTE); + validateAfter = validateInput("validateAfter", validateAfter, HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY); this.httpClient = OptimizelyHttpClient.builder() .withMaxTotalConnections(maxConnections) .withMaxPerRoute(connectionsPerRoute) .withValidateAfterInactivity(validateAfter) // infrequent event discards observed. staled connections force-closed after a long idle time. .withEvictIdleConnections(1L, TimeUnit.MINUTES) + // enable retry on POST + .withRetryHandler(new DefaultHttpRequestRetryHandler(3, true)) .build(); } @@ -310,9 +311,9 @@ public static class Builder { int queueCapacity = PropertyUtils.getInteger(CONFIG_QUEUE_CAPACITY, DEFAULT_QUEUE_CAPACITY); int numWorkers = PropertyUtils.getInteger(CONFIG_NUM_WORKERS, DEFAULT_NUM_WORKERS); - int maxTotalConnections = PropertyUtils.getInteger(CONFIG_MAX_CONNECTIONS, DEFAULT_MAX_CONNECTIONS); - int maxPerRoute = PropertyUtils.getInteger(CONFIG_MAX_PER_ROUTE, DEFAULT_MAX_PER_ROUTE); - int validateAfterInactivity = PropertyUtils.getInteger(CONFIG_VALIDATE_AFTER_INACTIVITY, DEFAULT_VALIDATE_AFTER_INACTIVITY); + int maxTotalConnections = PropertyUtils.getInteger(CONFIG_MAX_CONNECTIONS, HttpClientUtils.DEFAULT_MAX_CONNECTIONS); + int maxPerRoute = PropertyUtils.getInteger(CONFIG_MAX_PER_ROUTE, HttpClientUtils.DEFAULT_MAX_PER_ROUTE); + int validateAfterInactivity = PropertyUtils.getInteger(CONFIG_VALIDATE_AFTER_INACTIVITY, HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY); private long closeTimeout = Long.MAX_VALUE; private TimeUnit closeTimeoutUnit = TimeUnit.MILLISECONDS; private OptimizelyHttpClient httpClient; diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java index 4667bec34..15a15d21e 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java @@ -16,27 +16,29 @@ */ package com.optimizely.ab; +import io.javalin.Javalin; +import org.apache.http.NoHttpResponseException; +import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.client.methods.RequestBuilder; import org.apache.http.conn.HttpHostConnectException; import org.apache.http.impl.client.CloseableHttpClient; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; +import org.junit.*; import java.io.IOException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import static com.optimizely.ab.OptimizelyHttpClient.builder; import static java.util.concurrent.TimeUnit.*; import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.*; public class OptimizelyHttpClientTest { - @Before public void setUp() { System.setProperty("https.proxyHost", "localhost"); @@ -51,7 +53,13 @@ public void tearDown() { @Test public void testDefaultConfiguration() { - OptimizelyHttpClient optimizelyHttpClient = builder().build(); + OptimizelyHttpClient.Builder builder = builder(); + assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.maxTotalConnections, 200); + assertEquals(builder.maxPerRoute, 20); + assertNull(builder.customRetryHandler); + + OptimizelyHttpClient optimizelyHttpClient = builder.build(); assertTrue(optimizelyHttpClient.getHttpClient() instanceof CloseableHttpClient); } @@ -101,4 +109,76 @@ public void testExecute() throws IOException { OptimizelyHttpClient optimizelyHttpClient = new OptimizelyHttpClient(mockHttpClient); assertTrue(optimizelyHttpClient.execute(httpUriRequest, responseHandler)); } + + @Test + public void testRetries() throws IOException { + // Javalin intercepts before proxy, so host and port should be set correct here + String host = "http://localhost"; + int port = 8000; + Javalin app = Javalin.create().start(port); + int maxFailures = 2; + + AtomicInteger callTimes = new AtomicInteger(); + app.get("/", ctx -> { + callTimes.addAndGet(1); + int count = callTimes.get(); + if (count < maxFailures) { + throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); + } else { + ctx.status(200).result("Success"); + } + }); + + OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().build()); + optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); + assertEquals(3, callTimes.get()); + } + + @Test + public void testRetriesWithCustom() throws IOException { + // Javalin intercepts before proxy, so host and port should be set correct here + String host = "http://localhost"; + int port = 8000; + Javalin app = Javalin.create().start(port); + int maxFailures = 2; + + AtomicInteger callTimes = new AtomicInteger(); + app.get("/", ctx -> { + callTimes.addAndGet(1); + int count = callTimes.get(); + if (count < maxFailures) { +// throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); + throw new IOException("TESTING CONNECTION FAILURE"); + +// ctx.status(500).result("TESTING Server Error"); + } else { + ctx.status(200).result("Success"); + } + }); + + HttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(3, true); + OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().withRetryHandler(retryHandler).build()); + + optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); + assertEquals(3, callTimes.get()); + } + +// +// @Test +// public void testRetriesWithCustom() throws IOException { +// CloseableHttpClient httpClient = mock(CloseableHttpClient.class); +// CloseableHttpResponse response = mock(CloseableHttpResponse.class); +// +// HttpRequestRetryHandler mockRetryHandler = spy(new DefaultHttpRequestRetryHandler(3, true)); +// when(mockRetryHandler.retryRequest(any(), any(), any())).thenReturn(true); +// +// OptimizelyHttpClient optimizelyHttpClient = OptimizelyHttpClient.builder().withRetryHandler(mockRetryHandler).build(); +// try { +// optimizelyHttpClient.execute(new HttpGet("https://example.com")); +// } catch(Exception e) { +// assert(e instanceof IOException); +// } +// verify(mockRetryHandler, times(3)).retryRequest(any(), any(), any()); +// } + } diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java index f87811b96..34a15ef8d 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java @@ -124,6 +124,7 @@ public void testBuilderWithCustomHttpClient() { AsyncEventHandler eventHandler = builder() .withOptimizelyHttpClient(customHttpClient) + // these params will be ignored when customHttpClient is injected .withMaxTotalConnections(1) .withMaxPerRoute(2) .withCloseTimeout(10, TimeUnit.SECONDS) @@ -134,6 +135,17 @@ public void testBuilderWithCustomHttpClient() { @Test public void testBuilderWithDefaultHttpClient() { + AsyncEventHandler.Builder builder = builder(); + assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.maxTotalConnections, 200); + assertEquals(builder.maxPerRoute, 20); + + AsyncEventHandler eventHandler = builder.build(); + assert(eventHandler.httpClient != null); + } + + @Test + public void testBuilderWithDefaultHttpClientAndCustomParams() { AsyncEventHandler eventHandler = builder() .withMaxTotalConnections(3) .withMaxPerRoute(4) From c7bc37920c540a65c74691a52df55eada5db9529 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 7 May 2024 11:47:43 -0700 Subject: [PATCH 2/6] reduce request drops with smaller validation time and retries --- core-httpclient-impl/README.md | 28 ++-- core-httpclient-impl/build.gradle | 11 +- .../com/optimizely/ab/HttpClientUtils.java | 2 +- .../optimizely/ab/OptimizelyHttpClient.java | 2 + .../ab/event/AsyncEventHandler.java | 2 +- .../ab/OptimizelyHttpClientTest.java | 142 +++++++++--------- 6 files changed, 96 insertions(+), 91 deletions(-) diff --git a/core-httpclient-impl/README.md b/core-httpclient-impl/README.md index 55eac460b..5781abb8d 100644 --- a/core-httpclient-impl/README.md +++ b/core-httpclient-impl/README.md @@ -106,24 +106,24 @@ The number of workers determines the number of threads the thread pool uses. ### Builder Methods The following builder methods can be used to custom configure the `AsyncEventHandler`. -|Method Name|Default Value| Description | -|---|---|----------------------------------------------------| -|`withQueueCapacity(int)`|10000| Queue size for pending logEvents | -|`withNumWorkers(int)`|2| Number of worker threads | -|`withMaxTotalConnections(int)`|200| Maximum number of connections | -|`withMaxPerRoute(int)`|20| Maximum number of connections per route | -|`withValidateAfterInactivity(int)`|5000| Time to maintain idle connections (in milliseconds) | +|Method Name|Default Value|Description| +|---|---|-----------------------------------------------| +|`withQueueCapacity(int)`|10000|Queue size for pending logEvents| +|`withNumWorkers(int)`|2|Number of worker threads| +|`withMaxTotalConnections(int)`|200|Maximum number of connections| +|`withMaxPerRoute(int)`|20|Maximum number of connections per route| +|`withValidateAfterInactivity(int)`|5000|Time to maintain idle connections (in milliseconds)| ### Advanced configuration The following properties can be set to override the default configuration. -|Property Name|Default Value| Description | -|---|---|----------------------------------------------------| -|**async.event.handler.queue.capacity**|10000| Queue size for pending logEvents | -|**async.event.handler.num.workers**|2| Number of worker threads | -|**async.event.handler.max.connections**|200| Maximum number of connections | -|**async.event.handler.event.max.per.route**|20| Maximum number of connections per route | -|**async.event.handler.validate.after**|5000| Time to maintain idle connections (in milliseconds) | +|Property Name|Default Value|Description| +|---|---|-----------------------------------------------| +|**async.event.handler.queue.capacity**|10000|Queue size for pending logEvents| +|**async.event.handler.num.workers**|2|Number of worker threads| +|**async.event.handler.max.connections**|200|Maximum number of connections| +|**async.event.handler.event.max.per.route**|20|Maximum number of connections per route| +|**async.event.handler.validate.after**|5000|Time to maintain idle connections (in milliseconds)| ## HttpProjectConfigManager diff --git a/core-httpclient-impl/build.gradle b/core-httpclient-impl/build.gradle index b44733ff4..8fad0a93e 100644 --- a/core-httpclient-impl/build.gradle +++ b/core-httpclient-impl/build.gradle @@ -1,14 +1,9 @@ dependencies { - compile project(':core-api') - + implementation project(':core-api') compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion + implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion - compile group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion - - testImplementation 'io.javalin:javalin:3.13.10' - testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2' - testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.2' - testImplementation 'io.rest-assured:rest-assured:4.4.0' + testImplementation 'org.mock-server:mockserver-netty:5.15.0' } task exhaustiveTest { diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java index 88cc775d6..bc697e642 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java @@ -26,7 +26,7 @@ public final class HttpClientUtils { public static final int CONNECTION_TIMEOUT_MS = 10000; public static final int CONNECTION_REQUEST_TIMEOUT_MS = 5000; public static final int SOCKET_TIMEOUT_MS = 10000; - public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 5000; + public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 1000; public static final int DEFAULT_MAX_CONNECTIONS = 200; public static final int DEFAULT_MAX_PER_ROUTE = 20; private static RequestConfig requestConfigWithTimeout; diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java index 0e026f3e4..5b515aea6 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java @@ -81,6 +81,8 @@ public static class Builder { // The maximum number of connections allowed for a route int maxPerRoute = HttpClientUtils.DEFAULT_MAX_PER_ROUTE; // Defines period of inactivity in milliseconds after which persistent connections must be re-validated prior to being leased to the consumer. + // If this is too long, it's expected to see more requests dropped on staled connections (dropped by the server or networks). + // We can configure retries (POST for AsyncEventDispatcher) to cover the staled connections. int validateAfterInactivity = HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY; // force-close the connection after this idle time (with 0, eviction is disabled by default) long evictConnectionIdleTimePeriod = 0; diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java index 7b550b2c5..2a9c10ec9 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java @@ -143,7 +143,7 @@ public AsyncEventHandler(int queueCapacity, .withValidateAfterInactivity(validateAfter) // infrequent event discards observed. staled connections force-closed after a long idle time. .withEvictIdleConnections(1L, TimeUnit.MINUTES) - // enable retry on POST + // enable retry on event POST (default: retry on GET only) .withRetryHandler(new DefaultHttpRequestRetryHandler(3, true)) .build(); } diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java index 15a15d21e..9b188aff6 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java @@ -16,27 +16,37 @@ */ package com.optimizely.ab; -import io.javalin.Javalin; -import org.apache.http.NoHttpResponseException; +import org.apache.http.HttpException; import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.ResponseHandler; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.client.methods.RequestBuilder; import org.apache.http.conn.HttpHostConnectException; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; +import org.apache.http.protocol.HttpContext; import org.junit.*; +import org.mockserver.integration.ClientAndServer; +import org.mockserver.model.ConnectionOptions; +import org.mockserver.model.HttpError; +import org.mockserver.model.HttpRequest; +import org.mockserver.model.HttpResponse; import java.io.IOException; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import static com.optimizely.ab.OptimizelyHttpClient.builder; import static java.util.concurrent.TimeUnit.*; import static org.junit.Assert.*; -import static org.mockito.Matchers.any; import static org.mockito.Mockito.*; +import static org.mockserver.model.HttpForward.forward; +import static org.mockserver.model.HttpRequest.request; +import static org.mockserver.model.HttpResponse.*; +import static org.mockserver.model.HttpResponse.response; +import static org.mockserver.verify.VerificationTimes.exactly; public class OptimizelyHttpClientTest { @Before @@ -111,74 +121,72 @@ public void testExecute() throws IOException { } @Test - public void testRetries() throws IOException { - // Javalin intercepts before proxy, so host and port should be set correct here - String host = "http://localhost"; - int port = 8000; - Javalin app = Javalin.create().start(port); - int maxFailures = 2; - - AtomicInteger callTimes = new AtomicInteger(); - app.get("/", ctx -> { - callTimes.addAndGet(1); - int count = callTimes.get(); - if (count < maxFailures) { - throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); - } else { - ctx.status(200).result("Success"); - } - }); + public void testRetriesWithCustomRetryHandler() throws IOException { - OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().build()); - optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); - assertEquals(3, callTimes.get()); - } + // [NOTE] Request retries are all handled inside HttpClient. Not easy for unit test. + // - "DefaultHttpRetryHandler" in HttpClient retries only with special types of Exceptions + // like "NoHttpResponseException", etc. + // Other exceptions (SocketTimeout, ProtocolException, etc.) all ignored. + // - Not easy to force the specific exception type in the low-level. + // - This test just validates custom retry handler injected ok by validating the number of retries. - @Test - public void testRetriesWithCustom() throws IOException { - // Javalin intercepts before proxy, so host and port should be set correct here - String host = "http://localhost"; - int port = 8000; - Javalin app = Javalin.create().start(port); - int maxFailures = 2; - - AtomicInteger callTimes = new AtomicInteger(); - app.get("/", ctx -> { - callTimes.addAndGet(1); - int count = callTimes.get(); - if (count < maxFailures) { -// throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); - throw new IOException("TESTING CONNECTION FAILURE"); - -// ctx.status(500).result("TESTING Server Error"); - } else { - ctx.status(200).result("Success"); + class CustomRetryHandler implements HttpRequestRetryHandler { + private final int maxRetries; + + public CustomRetryHandler(int maxRetries) { + this.maxRetries = maxRetries; } - }); - HttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(3, true); - OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().withRetryHandler(retryHandler).build()); + @Override + public boolean retryRequest(IOException exception, int executionCount, HttpContext context) { + // override to retry for any type of exceptions + return executionCount < maxRetries; + } + } - optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); - assertEquals(3, callTimes.get()); - } + int port = 9999; + ClientAndServer mockServer; + int retryCount; -// -// @Test -// public void testRetriesWithCustom() throws IOException { -// CloseableHttpClient httpClient = mock(CloseableHttpClient.class); -// CloseableHttpResponse response = mock(CloseableHttpResponse.class); -// -// HttpRequestRetryHandler mockRetryHandler = spy(new DefaultHttpRequestRetryHandler(3, true)); -// when(mockRetryHandler.retryRequest(any(), any(), any())).thenReturn(true); -// -// OptimizelyHttpClient optimizelyHttpClient = OptimizelyHttpClient.builder().withRetryHandler(mockRetryHandler).build(); -// try { -// optimizelyHttpClient.execute(new HttpGet("https://example.com")); -// } catch(Exception e) { -// assert(e instanceof IOException); -// } -// verify(mockRetryHandler, times(3)).retryRequest(any(), any(), any()); -// } + // default httpclient (retries enabled by default, but no retry for timeout connection) + mockServer = ClientAndServer.startClientAndServer(port); + mockServer + .when(request().withMethod("GET").withPath("/")) + .error(HttpError.error()); + + OptimizelyHttpClient clientDefault = OptimizelyHttpClient.builder() + .setTimeoutMillis(100) + .build(); + + try { + clientDefault.execute(new HttpGet("http://localhost:" + port)); + fail(); + } catch (Exception e) { + retryCount = mockServer.retrieveRecordedRequests(request()).length; + assertEquals(1, retryCount); + } + mockServer.stop(); + + // httpclient with custom retry handler (5 times retries for any request) + + mockServer = ClientAndServer.startClientAndServer(port); + mockServer + .when(request().withMethod("GET").withPath("/")) + .error(HttpError.error()); + + OptimizelyHttpClient clientWithRetries = OptimizelyHttpClient.builder() + .withRetryHandler(new CustomRetryHandler(5)) + .setTimeoutMillis(100) + .build(); + + try { + clientWithRetries.execute(new HttpGet("http://localhost:" + port)); + fail(); + } catch (Exception e) { + retryCount = mockServer.retrieveRecordedRequests(request()).length; + assertEquals(5, retryCount); + } + mockServer.stop(); + } } From 7879667656c4e4dbf700d102337fe207490dec4a Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 7 May 2024 11:59:05 -0700 Subject: [PATCH 3/6] clean up --- core-httpclient-impl/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core-httpclient-impl/README.md b/core-httpclient-impl/README.md index 5781abb8d..762acb31a 100644 --- a/core-httpclient-impl/README.md +++ b/core-httpclient-impl/README.md @@ -112,7 +112,7 @@ The following builder methods can be used to custom configure the `AsyncEventHan |`withNumWorkers(int)`|2|Number of worker threads| |`withMaxTotalConnections(int)`|200|Maximum number of connections| |`withMaxPerRoute(int)`|20|Maximum number of connections per route| -|`withValidateAfterInactivity(int)`|5000|Time to maintain idle connections (in milliseconds)| +|`withValidateAfterInactivity(int)`|1000|Time to maintain idle connections (in milliseconds)| ### Advanced configuration The following properties can be set to override the default configuration. @@ -123,7 +123,7 @@ The following properties can be set to override the default configuration. |**async.event.handler.num.workers**|2|Number of worker threads| |**async.event.handler.max.connections**|200|Maximum number of connections| |**async.event.handler.event.max.per.route**|20|Maximum number of connections per route| -|**async.event.handler.validate.after**|5000|Time to maintain idle connections (in milliseconds)| +|**async.event.handler.validate.after**|1000|Time to maintain idle connections (in milliseconds)| ## HttpProjectConfigManager @@ -243,4 +243,4 @@ Optimizely optimizely = OptimizelyFactory.newDefaultInstance(); to enable request batching to the Optimizely logging endpoint. By default, a maximum of 10 events are included in each batch for a maximum interval of 30 seconds. These parameters are configurable via systems properties or through the `OptimizelyFactory#setMaxEventBatchSize` and `OptimizelyFactory#setMaxEventBatchInterval` methods. - \ No newline at end of file + From dc7e90104e645a5df3ad4dc9c60661debcd1101e Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 7 May 2024 13:32:45 -0700 Subject: [PATCH 4/6] fix tests --- build.gradle | 25 ++++++++++--------- core-httpclient-impl/build.gradle | 2 -- .../ab/OptimizelyHttpClientTest.java | 2 +- .../ab/event/AsyncEventHandlerTest.java | 2 +- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/build.gradle b/build.gradle index b8405e39b..5af62a36b 100644 --- a/build.gradle +++ b/build.gradle @@ -94,21 +94,22 @@ configure(publishedProjects) { } dependencies { - compile group: 'commons-codec', name: 'commons-codec', version: commonCodecVersion + implementation group: 'commons-codec', name: 'commons-codec', version: commonCodecVersion - testCompile group: 'junit', name: 'junit', version: junitVersion - testCompile group: 'org.mockito', name: 'mockito-core', version: mockitoVersion - testCompile group: 'org.hamcrest', name: 'hamcrest-all', version: hamcrestVersion - testCompile group: 'com.google.guava', name: 'guava', version: guavaVersion + testImplementation group: 'junit', name: 'junit', version: junitVersion + testImplementation group: 'org.mockito', name: 'mockito-core', version: mockitoVersion + testImplementation group: 'org.hamcrest', name: 'hamcrest-all', version: hamcrestVersion + testImplementation group: 'com.google.guava', name: 'guava', version: guavaVersion // logging dependencies (logback) - testCompile group: 'ch.qos.logback', name: 'logback-classic', version: logbackVersion - testCompile group: 'ch.qos.logback', name: 'logback-core', version: logbackVersion - - testCompile group: 'com.google.code.gson', name: 'gson', version: gsonVersion - testCompile group: 'org.json', name: 'json', version: jsonVersion - testCompile group: 'com.googlecode.json-simple', name: 'json-simple', version: jsonSimpleVersion - testCompile group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: jacksonVersion + testImplementation group: 'ch.qos.logback', name: 'logback-classic', version: logbackVersion + testImplementation group: 'ch.qos.logback', name: 'logback-core', version: logbackVersion + testImplementation 'org.mock-server:mockserver-netty:5.1.1' + + testImplementation group: 'com.google.code.gson', name: 'gson', version: gsonVersion + testImplementation group: 'org.json', name: 'json', version: jsonVersion + testImplementation group: 'com.googlecode.json-simple', name: 'json-simple', version: jsonSimpleVersion + testImplementation group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: jacksonVersion } def docTitle = "Optimizely Java SDK" diff --git a/core-httpclient-impl/build.gradle b/core-httpclient-impl/build.gradle index 8fad0a93e..a81f4cc37 100644 --- a/core-httpclient-impl/build.gradle +++ b/core-httpclient-impl/build.gradle @@ -2,8 +2,6 @@ dependencies { implementation project(':core-api') compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion - - testImplementation 'org.mock-server:mockserver-netty:5.15.0' } task exhaustiveTest { diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java index 9b188aff6..d80a4f1ef 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java @@ -64,7 +64,7 @@ public void tearDown() { @Test public void testDefaultConfiguration() { OptimizelyHttpClient.Builder builder = builder(); - assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.validateAfterInactivity, 1000); assertEquals(builder.maxTotalConnections, 200); assertEquals(builder.maxPerRoute, 20); assertNull(builder.customRetryHandler); diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java index 34a15ef8d..19f1faba9 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java @@ -136,7 +136,7 @@ public void testBuilderWithCustomHttpClient() { @Test public void testBuilderWithDefaultHttpClient() { AsyncEventHandler.Builder builder = builder(); - assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.validateAfterInactivity, 1000); assertEquals(builder.maxTotalConnections, 200); assertEquals(builder.maxPerRoute, 20); From a4c149a8570488e168320e172e74858e92f29fc6 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 7 May 2024 13:52:20 -0700 Subject: [PATCH 5/6] clean up --- build.gradle | 25 ++++++++++++------------- core-httpclient-impl/build.gradle | 5 +++-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/build.gradle b/build.gradle index 5af62a36b..b8405e39b 100644 --- a/build.gradle +++ b/build.gradle @@ -94,22 +94,21 @@ configure(publishedProjects) { } dependencies { - implementation group: 'commons-codec', name: 'commons-codec', version: commonCodecVersion + compile group: 'commons-codec', name: 'commons-codec', version: commonCodecVersion - testImplementation group: 'junit', name: 'junit', version: junitVersion - testImplementation group: 'org.mockito', name: 'mockito-core', version: mockitoVersion - testImplementation group: 'org.hamcrest', name: 'hamcrest-all', version: hamcrestVersion - testImplementation group: 'com.google.guava', name: 'guava', version: guavaVersion + testCompile group: 'junit', name: 'junit', version: junitVersion + testCompile group: 'org.mockito', name: 'mockito-core', version: mockitoVersion + testCompile group: 'org.hamcrest', name: 'hamcrest-all', version: hamcrestVersion + testCompile group: 'com.google.guava', name: 'guava', version: guavaVersion // logging dependencies (logback) - testImplementation group: 'ch.qos.logback', name: 'logback-classic', version: logbackVersion - testImplementation group: 'ch.qos.logback', name: 'logback-core', version: logbackVersion - testImplementation 'org.mock-server:mockserver-netty:5.1.1' - - testImplementation group: 'com.google.code.gson', name: 'gson', version: gsonVersion - testImplementation group: 'org.json', name: 'json', version: jsonVersion - testImplementation group: 'com.googlecode.json-simple', name: 'json-simple', version: jsonSimpleVersion - testImplementation group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: jacksonVersion + testCompile group: 'ch.qos.logback', name: 'logback-classic', version: logbackVersion + testCompile group: 'ch.qos.logback', name: 'logback-core', version: logbackVersion + + testCompile group: 'com.google.code.gson', name: 'gson', version: gsonVersion + testCompile group: 'org.json', name: 'json', version: jsonVersion + testCompile group: 'com.googlecode.json-simple', name: 'json-simple', version: jsonSimpleVersion + testCompile group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: jacksonVersion } def docTitle = "Optimizely Java SDK" diff --git a/core-httpclient-impl/build.gradle b/core-httpclient-impl/build.gradle index a81f4cc37..e4cdd4b99 100644 --- a/core-httpclient-impl/build.gradle +++ b/core-httpclient-impl/build.gradle @@ -1,7 +1,8 @@ dependencies { - implementation project(':core-api') + compile project(':core-api') compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion - implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion + compile group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion + testCompile 'org.mock-server:mockserver-netty:5.1.1' } task exhaustiveTest { From ce447558b4f01904e0b0ae8f48a544422930eb60 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 7 May 2024 14:28:57 -0700 Subject: [PATCH 6/6] remove redundant mock --- .../test/java/com/optimizely/ab/odp/ODPEventManagerTest.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java b/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java index 8a7546300..0ade4652f 100644 --- a/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java +++ b/core-api/src/test/java/com/optimizely/ab/odp/ODPEventManagerTest.java @@ -51,11 +51,6 @@ public class ODPEventManagerTest { @Captor ArgumentCaptor payloadCaptor; - @Before - public void setup() { - mockApiManager = mock(ODPApiManager.class); - } - @Test public void logAndDiscardEventWhenEventManagerIsNotRunning() { ODPConfig odpConfig = new ODPConfig("key", "host", null);