Revert "Revert "Revert "Revert "Enforce CC timeouts in Orchestrator 4""""

9 files changed, 151 insertions, 44 deletions

diff --git a/jaxrs_client_utils/pom.xml b/jaxrs_client_utils/pom.xml
index e3de5b8163d..43fbc66a9e6 100644
--- a/jaxrs_client_utils/pom.xml
+++ b/jaxrs_client_utils/pom.xml
@@ -18,6 +18,12 @@
   <dependencies>
     <dependency>
       <groupId>com.yahoo.vespa</groupId>
+      <artifactId>vespajlib</artifactId>
+      <version>${project.version}</version>
+      <scope>provided</scope>
+    </dependency>
+    <dependency>
+      <groupId>com.yahoo.vespa</groupId>
       <artifactId>application-model</artifactId>
       <version>${project.version}</version>
       <scope>provided</scope>
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..27d7024b9bd 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<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 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<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
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);
@@ -67,10 +53,16 @@ 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);
-        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<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) {
-                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<T> 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<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 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<JaxRsClientFactory.Params<TestJaxRsApi>> 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<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));
+        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())));
     }
 
     @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<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));
+    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);
     }
 }