Drop the http.reuseConnectionAfter503 configuration parameter We probably gain nothing by reusing http connections in this case. Change-Id: I2c481d20fdbae6796848bed2a15dd1662cc094a3
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java index 1d85210..ad6a7cc 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
@@ -477,8 +477,6 @@ static final String MAX_TRIES_KEY = "maxTries"; static final String RETRY_INTERVAL_KEY = "retryInterval"; static final String THREAD_POOL_SIZE_KEY = "threadPoolSize"; - static final String REUSE_CONNECTION_AFTER_503_KEY = "reuseConnectionAfter503"; - static final boolean DEFAULT_REUSE_CONNECTION_AFTER_503 = true; private final String user; private final String password; @@ -487,7 +485,6 @@ private final int maxTries; private final Duration retryInterval; private final int threadPoolSize; - private final boolean reuseConnectionAfter503; private Http(Config cfg) { user = Strings.nullToEmpty(cfg.getString(HTTP_SECTION, null, USER_KEY)); @@ -497,9 +494,6 @@ maxTries = getMaxTries(cfg, HTTP_SECTION, MAX_TRIES_KEY, DEFAULT_MAX_TRIES); retryInterval = getDuration(cfg, HTTP_SECTION, RETRY_INTERVAL_KEY, DEFAULT_RETRY_INTERVAL); threadPoolSize = getInt(cfg, HTTP_SECTION, THREAD_POOL_SIZE_KEY, DEFAULT_THREAD_POOL_SIZE); - reuseConnectionAfter503 = - cfg.getBoolean( - HTTP_SECTION, REUSE_CONNECTION_AFTER_503_KEY, DEFAULT_REUSE_CONNECTION_AFTER_503); } public String user() { @@ -529,10 +523,6 @@ public int threadPoolSize() { return threadPoolSize; } - - public boolean reuseConnectionAfter503() { - return reuseConnectionAfter503; - } } /** Common parameters to cache, event, index and websession */
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProvider.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProvider.java index fc90a10..93d324c 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProvider.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProvider.java
@@ -101,8 +101,7 @@ private ConnectionReuseStrategy customConnectionReuseStrategy() { return (response, context) -> { - if (response.getStatusLine().getStatusCode() == SC_SERVICE_UNAVAILABLE - && !cfg.http().reuseConnectionAfter503()) { + if (response.getStatusLine().getStatusCode() == SC_SERVICE_UNAVAILABLE) { return false; } return DefaultConnectionReuseStrategy.INSTANCE.keepAlive(response, context);
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index ca5ab18..36d8a09 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -268,22 +268,6 @@ Value is expressed in Gerrit time values as in [websession.cleanupInterval](#websessioncleanupInterval). When not specified, the default value is set to 10 seconds. -```http.reuseConnectionAfter503``` -: Whether to reuse the HTTP connection to the peer instance after receiving a - 503 (Service Unavailable) response over it. When set to `false`, the plugin - will close the connection immediately on a 503 response and open a fresh one - for the next retry, which can help recover from situations where a peer has - transiently become unavailable without properly closing existing connections. - When not specified, the default value is `true`. - -```http.reuseConnectionAfter503``` -: Whether to reuse the HTTP connection to the peer instance after receiving a - 503 (Service Unavailable) response over it. When set to `false`, the plugin - will close the connection immediately on a 503 response and open a fresh one - for the next retry, which can help recover from situations where a peer has - transiently become unavailable without properly closing existing connections. - When not specified, the default value is `true`. - ```http.threadPoolSize``` : Maximum number of threads used to execute REST calls towards target instances.
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProviderConnectionReuseTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProviderConnectionReuseTest.java index 3bd3b14..5954629 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProviderConnectionReuseTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/HttpClientProviderConnectionReuseTest.java
@@ -24,11 +24,11 @@ import com.ericsson.gerrit.plugins.highavailability.Configuration; import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.time.Duration; -import java.util.List; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.mockito.Answers; @@ -39,52 +39,39 @@ private static final Duration TIMEOUT = Duration.ofMillis(1000); @Rule public WireMockRule wireMock = new WireMockRule(0); + private Configuration cfg; + private TestableHttpClientProvider provider; - private Configuration mockConfig(boolean reuseConnectionAfter503) { - Configuration cfg = mock(Configuration.class, Answers.RETURNS_DEEP_STUBS); + @Before + public void setUp() { + cfg = mock(Configuration.class, Answers.RETURNS_DEEP_STUBS); when(cfg.http().user()).thenReturn(""); when(cfg.http().password()).thenReturn(""); when(cfg.http().connectionTimeout()).thenReturn(TIMEOUT); when(cfg.http().socketTimeout()).thenReturn(TIMEOUT); - when(cfg.http().reuseConnectionAfter503()).thenReturn(reuseConnectionAfter503); - return cfg; + + provider = new TestableHttpClientProvider(cfg); } @Test - public void connectionIsReturnedToPoolAfter503WhenReuseIsEnabled() throws Exception { + public void connectionIsDiscardedAfter503() throws Exception { wireMock.givenThat(get(urlEqualTo(ENDPOINT)).willReturn(aResponse().withStatus(503))); - TestableHttpClientProvider provider = new TestableHttpClientProvider(mockConfig(true)); try (CloseableHttpClient client = provider.get()) { executeConsuming(client); - // Connection must be back in the pool (available == 1) because reuseConnectionAfter503=true - assertThat(provider.connectionManager().getTotalStats().getAvailable()).isEqualTo(1); - } - } - - @Test - public void connectionIsDiscardedAfter503WhenReuseIsDisabled() throws Exception { - wireMock.givenThat(get(urlEqualTo(ENDPOINT)).willReturn(aResponse().withStatus(503))); - - TestableHttpClientProvider provider = new TestableHttpClientProvider(mockConfig(false)); - try (CloseableHttpClient client = provider.get()) { - executeConsuming(client); - // Connection must have been discarded (available == 0) because reuseConnectionAfter503=false + // Connection must have been discarded (available == 0) assertThat(provider.connectionManager().getTotalStats().getAvailable()).isEqualTo(0); } } @Test - public void connectionIsReturnedToPoolAfter500RegardlessOfReuseOption() throws Exception { + public void connectionIsReturnedToPoolAfter500() throws Exception { wireMock.givenThat(get(urlEqualTo(ENDPOINT)).willReturn(aResponse().withStatus(500))); - for (boolean reuseFlag : List.of(true, false)) { - TestableHttpClientProvider provider = new TestableHttpClientProvider(mockConfig(reuseFlag)); - try (CloseableHttpClient client = provider.get()) { - executeConsuming(client); - // 500 is not 503 — the reuseConnectionAfter503 option must have no effect - assertThat(provider.connectionManager().getTotalStats().getAvailable()).isEqualTo(1); - } + try (CloseableHttpClient client = provider.get()) { + executeConsuming(client); + // 500 is not 503 — connection returned to the pool + assertThat(provider.connectionManager().getTotalStats().getAvailable()).isEqualTo(1); } }