From d7e75c63c3c25474e825488c0daaa328e27472d8 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Mon, 29 Oct 2018 10:50:07 +0100 Subject: Revert "Revert "Enforce CC timeouts in Orchestrator 2"" --- .../vespa/jaxrs/client/JaxRsClientFactory.java | 44 ++++++++++++++++++++ .../yahoo/vespa/jaxrs/client/JaxRsStrategy.java | 4 ++ .../yahoo/vespa/jaxrs/client/JaxRsTimeouts.java | 22 ++++++++++ .../jaxrs/client/JerseyJaxRsClientFactory.java | 28 +++++-------- .../vespa/jaxrs/client/LegacyJaxRsTimeouts.java | 26 ++++++++++++ .../vespa/jaxrs/client/RetryingJaxRsStrategy.java | 14 ++++++- .../yahoo/vespa/jaxrs/client/HttpPatchTest.java | 3 +- .../jaxrs/client/RetryingJaxRsStrategyTest.java | 48 +++++++++++----------- 8 files changed, 145 insertions(+), 44 deletions(-) create mode 100644 jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java create mode 100644 jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java (limited to 'jaxrs_client_utils/src') 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 d004ac3af45..78076bdc04b 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,11 +3,55 @@ 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 { + private final Class apiClass; + private final URI uri; + + private Duration connectTimeout = Duration.ofSeconds(30); + private Duration readTimeout = Duration.ofSeconds(30); + + public Params(Class apiClass, URI uri) { + this.apiClass = apiClass; + this.uri = uri; + } + + public Class 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 createClient(Params params) { + throw new UnsupportedOperationException("Not implemented"); + } + T createClient(Class 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 72af76fe54c..cd7d8684cbc 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,4 +11,8 @@ import java.util.function.Function; */ public interface JaxRsStrategy { R apply(final Function function) throws IOException; + + default R apply(final Function 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 new file mode 100644 index 00000000000..6758d6f94d6 --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java @@ -0,0 +1,22 @@ +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 9321f8e290d..8aa880fb0e4 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,34 +23,20 @@ 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(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS); + this(null, null, null); } 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); @@ -66,11 +52,17 @@ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { this.client = builder.build(); } + @Override + public T createClient(Params 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 createClient(Class apiClass, HostName hostName, int port, String pathPrefix, String scheme) { UriBuilder uriBuilder = UriBuilder.fromPath(pathPrefix).host(hostName.s()).port(port).scheme(scheme); - WebTarget target = client.target(uriBuilder); - return WebResourceFactory.newResource(apiClass, target); + return createClient(new Params<>(apiClass, uriBuilder.build())); } - } 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 new file mode 100644 index 00000000000..3f2139f6bf0 --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java @@ -0,0 +1,26 @@ +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 65b302ef4ff..4c97147d61e 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,7 +4,9 @@ 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; @@ -65,11 +67,21 @@ public class RetryingJaxRsStrategy implements JaxRsStrategy { @Override public R apply(final Function function) throws IOException { + return apply(function, new LegacyJaxRsTimeouts()); + } + + + @Override + public R apply(final Function function, JaxRsTimeouts timeouts) throws IOException { ProcessingException sampleException = null; for (int i = 0; i < maxIterations; ++i) { for (final HostName hostName : hostNames) { - final T jaxRsClient = jaxRsClientFactory.createClient(apiClass, hostName, port, pathPrefix, scheme); + URI uri = UriBuilder.fromPath(pathPrefix).port(port).scheme(scheme).host(hostName.s()).build(); + JaxRsClientFactory.Params params = new JaxRsClientFactory.Params<>(apiClass, uri); + params.setConnectTimeout(timeouts.getConnectTimeoutOrThrow()); + params.setReadTimeout(timeouts.getReadTimeoutOrThrow()); + final T jaxRsClient = jaxRsClientFactory.createClient(params); 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 63e2b814c24..8161602cdac 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,8 +74,7 @@ public class HttpPatchTest extends JerseyTest { final JaxRsStrategy 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 e31920febd6..dbe886b7896 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,7 +5,10 @@ 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; @@ -15,24 +18,26 @@ 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.equalTo; -import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.hasItem; 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> paramsCaptor; + @Path(API_PATH) private interface TestJaxRsApi { @GET @@ -53,8 +58,7 @@ public class RetryingJaxRsStrategyTest { @Before public void setup() { - when(jaxRsClientFactory.createClient(eq(TestJaxRsApi.class), any(HostName.class), anyInt(), anyString(), anyString())) - .thenReturn(mockApi); + when(jaxRsClientFactory.createClient(any())).thenReturn(mockApi); } @Test @@ -63,11 +67,12 @@ public class RetryingJaxRsStrategyTest { verify(mockApi, times(1)).doSomething(); - // Check that one of the supplied hosts is contacted. - final ArgumentCaptor 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)); + verify(jaxRsClientFactory, times(1)).createClient(paramsCaptor.capture()); + JaxRsClientFactory.Params 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()))); } @Test @@ -99,10 +104,10 @@ public class RetryingJaxRsStrategyTest { @Test public void testRetryLoopsOverAvailableServers() throws Exception { when(mockApi.doSomething()) - .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")) + .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")) .thenReturn("a response"); jaxRsStrategy.apply(TestJaxRsApi::doSomething); @@ -142,12 +147,9 @@ public class RetryingJaxRsStrategyTest { verifyAllServersContacted(jaxRsClientFactory); } - private static void verifyAllServersContacted( - final JaxRsClientFactory jaxRsClientFactory) { - final ArgumentCaptor 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 actualServerHostsContacted = new HashSet<>(hostNameCaptor.getAllValues()); - assertThat(actualServerHostsContacted, equalTo(SERVER_HOSTS)); + private void verifyAllServersContacted(final JaxRsClientFactory jaxRsClientFactory) { + verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())).createClient(paramsCaptor.capture()); + final Set> actualServerHostsContacted = new HashSet<>(paramsCaptor.getAllValues()); + assertEquals(actualServerHostsContacted.stream().map(x -> new HostName(x.uri().getHost())).collect(Collectors.toSet()), SERVER_HOSTS); } } -- cgit v1.2.3