diff options
author | Harald Musum <musum@oath.com> | 2018-10-31 21:05:12 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-10-31 21:05:12 +0100 |
commit | 875c0cb04d0f0450bfb68d8e548fcaea6022bd49 (patch) | |
tree | 9c3b311dcbc9f7569ffc56939273f3f26831b311 /jaxrs_client_utils/src | |
parent | 3db4288215479f38f7db9c1ddeeceb69a8ff93e5 (diff) |
Revert "Enforce CC timeouts in Orchestrator [4]"
Diffstat (limited to 'jaxrs_client_utils/src')
8 files changed, 44 insertions, 145 deletions
diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java index 27d7024b9bd..d004ac3af45 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java @@ -3,55 +3,11 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; -import java.net.URI; -import java.time.Duration; - /** * Interface for creating a JAX-RS client API instance for a single server endpoint. * * @author bakksjo */ public interface JaxRsClientFactory { - class Params<T> { - private final Class<T> apiClass; - private final URI uri; - - private Duration connectTimeout = Duration.ofSeconds(30); - private Duration readTimeout = Duration.ofSeconds(30); - - public Params(Class<T> apiClass, URI uri) { - this.apiClass = apiClass; - this.uri = uri; - } - - public Class<T> apiClass() { - return apiClass; - } - - public URI uri() { - return uri; - } - - public void setConnectTimeout(Duration timeout) { - this.connectTimeout = timeout; - } - - public Duration connectTimeout() { - return connectTimeout; - } - - public void setReadTimeout(Duration timeout) { - readTimeout = timeout; - } - - public Duration readTimeout() { - return readTimeout; - } - } - - default <T> T createClient(Params<T> params) { - return createClient(params.apiClass, new HostName(params.uri.getHost()), params.uri.getPort(), params.uri.getPath(), params.uri.getScheme()); - } - <T> T createClient(Class<T> apiClass, HostName hostName, int port, String pathPrefix, String scheme); } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java index cd7d8684cbc..72af76fe54c 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java @@ -11,8 +11,4 @@ import java.util.function.Function; */ public interface JaxRsStrategy<T> { <R> R apply(final Function<T, R> function) throws IOException; - - default <R> R apply(final Function<T, R> function, JaxRsTimeouts timeouts) throws IOException { - return apply(function); - } } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java deleted file mode 100644 index 6758d6f94d6..00000000000 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java +++ /dev/null @@ -1,22 +0,0 @@ -package com.yahoo.vespa.jaxrs.client; - -import java.time.Duration; - -/** - * @author hakonhall - */ -public interface JaxRsTimeouts { - /** - * The connect timeout, which must be at least 1ms. - * - * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. - */ - Duration getConnectTimeoutOrThrow(); - - /** - * The read timeout, which must be at least 1ms. - * - * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. - */ - Duration getReadTimeoutOrThrow(); -} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java index 8aa880fb0e4..9321f8e290d 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java @@ -23,20 +23,34 @@ import java.util.Collections; */ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { + private static final int DEFAULT_CONNECT_TIMEOUT_MS = 30000; + private static final int DEFAULT_READ_TIMEOUT_MS = 30000; + // Client is a heavy-weight object with a finalizer so we create only one and re-use it private final Client client; public JerseyJaxRsClientFactory() { - this(null, null, null); + this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS); } public JerseyJaxRsClientFactory(SSLContext sslContext, HostnameVerifier hostnameVerifier, String userAgent) { + this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS, sslContext, hostnameVerifier, userAgent); + } + + public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs) { + this(connectTimeoutMs, readTimeoutMs, null, null, null); + } + + public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs, SSLContext sslContext, + HostnameVerifier hostnameVerifier, String userAgent) { /* * Configure client with some workarounds for HTTP/JAX-RS/Jersey issues. See: * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/ClientProperties.html#SUPPRESS_HTTP_COMPLIANCE_VALIDATION * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/HttpUrlConnectorProvider.html#SET_METHOD_WORKAROUND */ ClientBuilder builder = ClientBuilder.newBuilder() + .property(ClientProperties.CONNECT_TIMEOUT, connectTimeoutMs) + .property(ClientProperties.READ_TIMEOUT, readTimeoutMs) .property(ClientProperties.SUPPRESS_HTTP_COMPLIANCE_VALIDATION, true) // Allow empty PUT. TODO: Fix API. .property(HttpUrlConnectorProvider.SET_METHOD_WORKAROUND, true) // Allow e.g. PATCH method. .property(ClientProperties.FOLLOW_REDIRECTS, true); @@ -53,16 +67,10 @@ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { } @Override - public <T> T createClient(Params<T> params) { - WebTarget target = client.target(params.uri()); - target.property(ClientProperties.CONNECT_TIMEOUT, (int) params.connectTimeout().toMillis()); - target.property(ClientProperties.READ_TIMEOUT, (int) params.readTimeout().toMillis()); - return WebResourceFactory.newResource(params.apiClass(), target); - } - - @Override public <T> T createClient(Class<T> apiClass, HostName hostName, int port, String pathPrefix, String scheme) { UriBuilder uriBuilder = UriBuilder.fromPath(pathPrefix).host(hostName.s()).port(port).scheme(scheme); - return createClient(new Params<>(apiClass, uriBuilder.build())); + WebTarget target = client.target(uriBuilder); + return WebResourceFactory.newResource(apiClass, target); } + } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java deleted file mode 100644 index 3f2139f6bf0..00000000000 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java +++ /dev/null @@ -1,26 +0,0 @@ -package com.yahoo.vespa.jaxrs.client; - -import java.time.Duration; - -/** - * Legacy defaults for timeouts. - * - * Clients should instead define their own JaxRsTimeouts tailored to their use-case. - * - * @author hakonhall - */ -// Immutable -public class LegacyJaxRsTimeouts implements JaxRsTimeouts { - private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(30); - private static final Duration READ_TIMEOUT = Duration.ofSeconds(30); - - @Override - public Duration getConnectTimeoutOrThrow() { - return CONNECT_TIMEOUT; - } - - @Override - public Duration getReadTimeoutOrThrow() { - return READ_TIMEOUT; - } -} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java index 4c97147d61e..65b302ef4ff 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java @@ -4,9 +4,7 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; import javax.ws.rs.ProcessingException; -import javax.ws.rs.core.UriBuilder; import java.io.IOException; -import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -67,21 +65,11 @@ public class RetryingJaxRsStrategy<T> implements JaxRsStrategy<T> { @Override public <R> R apply(final Function<T, R> function) throws IOException { - return apply(function, new LegacyJaxRsTimeouts()); - } - - - @Override - public <R> R apply(final Function<T, R> function, JaxRsTimeouts timeouts) throws IOException { ProcessingException sampleException = null; for (int i = 0; i < maxIterations; ++i) { for (final HostName hostName : hostNames) { - URI uri = UriBuilder.fromPath(pathPrefix).port(port).scheme(scheme).host(hostName.s()).build(); - JaxRsClientFactory.Params<T> params = new JaxRsClientFactory.Params<>(apiClass, uri); - params.setConnectTimeout(timeouts.getConnectTimeoutOrThrow()); - params.setReadTimeout(timeouts.getReadTimeoutOrThrow()); - final T jaxRsClient = jaxRsClientFactory.createClient(params); + final T jaxRsClient = jaxRsClientFactory.createClient(apiClass, hostName, port, pathPrefix, scheme); try { return function.apply(jaxRsClient); } catch (ProcessingException e) { diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java index 8161602cdac..63e2b814c24 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java @@ -74,7 +74,8 @@ public class HttpPatchTest extends JerseyTest { final JaxRsStrategy<TestResourceApi> client = factory.apiNoRetries(TestResourceApi.class, apiPath); final String responseBody; - responseBody = client.apply(api -> api.doPatch(REQUEST_BODY)); + responseBody = client.apply(api -> + api.doPatch(REQUEST_BODY)); assertThat(testResourceSingleton.invocation.get(60, TimeUnit.SECONDS), is(REQUEST_BODY)); assertThat(responseBody, is(REQUEST_BODY)); diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java index dbe886b7896..e31920febd6 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java @@ -5,10 +5,7 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.defaults.Defaults; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -import org.mockito.runners.MockitoJUnitRunner; import org.mockito.stubbing.OngoingStubbing; import javax.ws.rs.GET; @@ -18,26 +15,24 @@ import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.Set; -import java.util.stream.Collectors; -import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@RunWith(MockitoJUnitRunner.class) public class RetryingJaxRsStrategyTest { private static final String API_PATH = "/"; - @Captor - ArgumentCaptor<JaxRsClientFactory.Params<TestJaxRsApi>> paramsCaptor; - @Path(API_PATH) private interface TestJaxRsApi { @GET @@ -58,7 +53,8 @@ public class RetryingJaxRsStrategyTest { @Before public void setup() { - when(jaxRsClientFactory.createClient(any())).thenReturn(mockApi); + when(jaxRsClientFactory.createClient(eq(TestJaxRsApi.class), any(HostName.class), anyInt(), anyString(), anyString())) + .thenReturn(mockApi); } @Test @@ -67,12 +63,11 @@ public class RetryingJaxRsStrategyTest { verify(mockApi, times(1)).doSomething(); - verify(jaxRsClientFactory, times(1)).createClient(paramsCaptor.capture()); - JaxRsClientFactory.Params<TestJaxRsApi> params = paramsCaptor.getValue(); - assertEquals(REST_PORT, params.uri().getPort()); - assertEquals(API_PATH, params.uri().getPath()); - assertEquals("http", params.uri().getScheme()); - assertThat(SERVER_HOSTS, hasItem(new HostName(params.uri().getHost()))); + // Check that one of the supplied hosts is contacted. + final ArgumentCaptor<HostName> hostNameCaptor = ArgumentCaptor.forClass(HostName.class); + verify(jaxRsClientFactory, times(1)) + .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); + assertThat(SERVER_HOSTS.contains(hostNameCaptor.getValue()), is(true)); } @Test @@ -104,10 +99,10 @@ public class RetryingJaxRsStrategyTest { @Test public void testRetryLoopsOverAvailableServers() throws Exception { when(mockApi.doSomething()) - .thenThrow(new ProcessingException("Fake socket timeout 1 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 2 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 3 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 4 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 1 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 2 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 3 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 4 induced by test")) .thenReturn("a response"); jaxRsStrategy.apply(TestJaxRsApi::doSomething); @@ -147,9 +142,12 @@ public class RetryingJaxRsStrategyTest { verifyAllServersContacted(jaxRsClientFactory); } - private void verifyAllServersContacted(final JaxRsClientFactory jaxRsClientFactory) { - verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())).createClient(paramsCaptor.capture()); - final Set<JaxRsClientFactory.Params<TestJaxRsApi>> actualServerHostsContacted = new HashSet<>(paramsCaptor.getAllValues()); - assertEquals(actualServerHostsContacted.stream().map(x -> new HostName(x.uri().getHost())).collect(Collectors.toSet()), SERVER_HOSTS); + private static void verifyAllServersContacted( + final JaxRsClientFactory jaxRsClientFactory) { + final ArgumentCaptor<HostName> hostNameCaptor = ArgumentCaptor.forClass(HostName.class); + verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())) + .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); + final Set<HostName> actualServerHostsContacted = new HashSet<>(hostNameCaptor.getAllValues()); + assertThat(actualServerHostsContacted, equalTo(SERVER_HOSTS)); } } |