diff options
author | Håkon Hallingstad <hakon@verizonmedia.com> | 2019-11-18 10:53:50 +0100 |
---|---|---|
committer | Håkon Hallingstad <hakon@verizonmedia.com> | 2019-11-18 10:53:50 +0100 |
commit | 4f8c813ee2473509e297b6eddc3e35af758546e0 (patch) | |
tree | 9f399b40b36bcf155749d3d777cc5d309ad90b7a /node-admin | |
parent | a179b9cf1d89e371bae93ac456edeeca9c0c2b63 (diff) |
Copy a default RequestConfig instead of the request RequestConfig which is null
Diffstat (limited to 'node-admin')
4 files changed, 67 insertions, 30 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java index e6d719e66bb..0fa7dbfacae 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java @@ -23,11 +23,11 @@ public interface ConfigServerApi extends AutoCloseable { } <T> T get(String path, Class<T> wantedReturnType); + <T> T get(String path, Class<T> wantedReturnType, Params params); <T> T post(String path, Object bodyJsonPojo, Class<T> wantedReturnType); <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType); - <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType, Params params); <T> T patch(String path, Object bodyJsonPojo, Class<T> wantedReturnType); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java index 53c4bac5f6c..4e6b3ed93a5 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java @@ -49,6 +49,11 @@ import java.util.logging.Logger; public class ConfigServerApiImpl implements ConfigServerApi { private static final Logger logger = Logger.getLogger(ConfigServerApiImpl.class.getName()); + private static final RequestConfig DEFAULT_REQUEST_CONFIG = RequestConfig.custom() + .setConnectionRequestTimeout(1_000) // connection from connection manager + .setConnectTimeout(10_000) // establishment of connection + .setSocketTimeout(10_000) // waiting for data + .build(); private final ObjectMapper mapper = new ObjectMapper(); @@ -140,9 +145,10 @@ public class ConfigServerApiImpl implements ConfigServerApi { @Override public <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType, Params paramsOrNull) { + Optional<RequestConfig> requestConfigOverride = getRequestConfigOverride(paramsOrNull); return tryAllConfigServers(configServer -> { HttpPut put = new HttpPut(configServer.resolve(path)); - setRequestConfigOverride(paramsOrNull, put); + requestConfigOverride.ifPresent(put::setConfig); setContentTypeToApplicationJson(put); if (bodyJsonPojo.isPresent()) { put.setEntity(new StringEntity(mapper.writeValueAsString(bodyJsonPojo.get()))); @@ -169,8 +175,17 @@ public class ConfigServerApiImpl implements ConfigServerApi { @Override public <T> T get(String path, Class<T> wantedReturnType) { - return tryAllConfigServers(configServer -> - new HttpGet(configServer.resolve(path)), wantedReturnType); + return get(path, wantedReturnType, null); + } + + @Override + public <T> T get(String path, Class<T> wantedReturnType, Params paramsOrNull) { + Optional<RequestConfig> requestConfig = getRequestConfigOverride(paramsOrNull); + return tryAllConfigServers(configServer -> { + HttpGet get = new HttpGet(configServer.resolve(path)); + requestConfig.ifPresent(get::setConfig); + return get; + }, wantedReturnType); } @Override @@ -210,31 +225,25 @@ public class ConfigServerApiImpl implements ConfigServerApi { // Have experienced hang in socket read, which may have been because of // system defaults, therefore set explicit timeouts. - RequestConfig defaultRequestConfig = RequestConfig.custom() - .setConnectionRequestTimeout(1_000) // connection from connection manager - .setConnectTimeout(10_000) // establishment of connection - .setSocketTimeout(10_000) // waiting for data - .build(); - return HttpClientBuilder.create() - .setDefaultRequestConfig(defaultRequestConfig) + .setDefaultRequestConfig(DEFAULT_REQUEST_CONFIG) .disableAutomaticRetries() .setUserAgent("node-admin") .setConnectionManager(cm) .build(); } - private static void setRequestConfigOverride(Params paramsOrNull, HttpRequestBase request) { - if (paramsOrNull == null) return; + private static Optional<RequestConfig> getRequestConfigOverride(Params paramsOrNull) { + if (paramsOrNull == null) return Optional.empty(); - RequestConfig.Builder builder = RequestConfig.copy(request.getConfig()); + RequestConfig.Builder builder = RequestConfig.copy(DEFAULT_REQUEST_CONFIG); paramsOrNull.getConnectionTimeout().ifPresent(connectionTimeout -> { builder.setConnectTimeout((int) connectionTimeout.toMillis()); builder.setSocketTimeout((int) connectionTimeout.toMillis()); }); - request.setConfig(builder.build()); + return Optional.of(builder.build()); } // Shuffle config server URIs to balance load diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java index 7e860bfb66b..ef91e9bf81b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java @@ -14,8 +14,8 @@ import java.net.SocketTimeoutException; @SuppressWarnings("serial") public class ConnectionException extends ConvergenceException { - private ConnectionException(String message) { - super(message); + private ConnectionException(String message, Throwable cause) { + super(message, cause); } /** @@ -24,7 +24,7 @@ public class ConnectionException extends ConvergenceException { */ public static RuntimeException handleException(String prefix, Throwable t) { if (isKnownConnectionException(t)) - return new ConnectionException(prefix + t.getMessage()); + return new ConnectionException(prefix + t.getMessage(), t); return new RuntimeException(prefix, t); } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java index 1ed3e5729e5..1eee377a845 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java @@ -15,15 +15,19 @@ import org.junit.Test; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; +import java.net.SocketTimeoutException; import java.net.URI; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Arrays; import java.util.List; +import java.util.Optional; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; import static org.hamcrest.junit.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -37,6 +41,9 @@ import static org.mockito.Mockito.when; */ public class ConfigServerApiImplTest { + private static final int FAIL_RETURN_CODE = 100000; + private static final int TIMEOUT_RETURN_CODE = 100001; + @JsonIgnoreProperties(ignoreUnknown = true) public static class TestPojo { @JsonProperty("foo") @@ -50,7 +57,7 @@ public class ConfigServerApiImplTest { private final List<URI> configServers = Arrays.asList(URI.create(uri1), URI.create(uri2)); private final StringBuilder mockLog = new StringBuilder(); - private ConfigServerApiImpl executor; + private ConfigServerApiImpl configServerApi; private int mockReturnCode = 200; @Before @@ -59,7 +66,11 @@ public class ConfigServerApiImplTest { when(httpMock.execute(any())).thenAnswer(invocationOnMock -> { HttpGet get = (HttpGet) invocationOnMock.getArguments()[0]; mockLog.append(get.getMethod()).append(" ").append(get.getURI()).append(" "); - if (mockReturnCode == 100000) throw new RuntimeException("FAIL"); + + switch (mockReturnCode) { + case FAIL_RETURN_CODE: throw new RuntimeException("FAIL"); + case TIMEOUT_RETURN_CODE: throw new SocketTimeoutException("read timed out"); + } BasicStatusLine statusLine = new BasicStatusLine(HttpVersion.HTTP_1_1, mockReturnCode, null); BasicHttpEntity entity = new BasicHttpEntity(); @@ -73,12 +84,12 @@ public class ConfigServerApiImplTest { return response; }); - executor = ConfigServerApiImpl.createForTestingWithClient(configServers, httpMock); + configServerApi = ConfigServerApiImpl.createForTestingWithClient(configServers, httpMock); } @Test public void testBasicParsingSingleServer() { - TestPojo answer = executor.get("/path", TestPojo.class); + TestPojo answer = configServerApi.get("/path", TestPojo.class); assertThat(answer.foo, is("bar")); assertLogStringContainsGETForAHost(); } @@ -88,7 +99,7 @@ public class ConfigServerApiImplTest { // Server is returning 400, no retries. mockReturnCode = 400; - TestPojo testPojo = executor.get("/path", TestPojo.class); + TestPojo testPojo = configServerApi.get("/path", TestPojo.class); assertEquals(testPojo.errorCode.intValue(), mockReturnCode); assertLogStringContainsGETForAHost(); } @@ -98,17 +109,34 @@ public class ConfigServerApiImplTest { // Server is returning 201, no retries. mockReturnCode = 201; - TestPojo testPojo = executor.get("/path", TestPojo.class); + TestPojo testPojo = configServerApi.get("/path", TestPojo.class); assertEquals(testPojo.errorCode.intValue(), mockReturnCode); assertLogStringContainsGETForAHost(); } @Test + public void testBasicSuccessWithCustomTimeouts() { + // Server is returning 201, no retries. + mockReturnCode = TIMEOUT_RETURN_CODE; + + var params = new ConfigServerApi.Params(); + params.setConnectionTimeout(Duration.ofSeconds(3)); + + try { + TestPojo testPojo = configServerApi.get("/path", TestPojo.class, params); + fail(); + } catch (ConnectionException e) { + assertNotNull(e.getCause()); + assertEquals("read timed out", e.getCause().getMessage()); + } + } + + @Test public void testRetries() { // Client is throwing exception, should be retries. - mockReturnCode = 100000; + mockReturnCode = FAIL_RETURN_CODE; try { - executor.get("/path", TestPojo.class); + configServerApi.get("/path", TestPojo.class); fail("Expected failure"); } catch (Exception e) { // ignore @@ -123,7 +151,7 @@ public class ConfigServerApiImplTest { // Client is throwing exception, should be retries. mockReturnCode = 503; try { - executor.get("/path", TestPojo.class); + configServerApi.get("/path", TestPojo.class); fail("Expected failure"); } catch (Exception e) { // ignore @@ -136,7 +164,7 @@ public class ConfigServerApiImplTest { public void testForbidden() { mockReturnCode = 403; try { - executor.get("/path", TestPojo.class); + configServerApi.get("/path", TestPojo.class); fail("Expected exception"); } catch (HttpException.ForbiddenException e) { // ignore @@ -149,7 +177,7 @@ public class ConfigServerApiImplTest { // Server is returning 404, special exception is thrown. mockReturnCode = 404; try { - executor.get("/path", TestPojo.class); + configServerApi.get("/path", TestPojo.class); fail("Expected exception"); } catch (HttpException.NotFoundException e) { // ignore @@ -161,7 +189,7 @@ public class ConfigServerApiImplTest { public void testConflict() { // Server is returning 409, no exception is thrown. mockReturnCode = 409; - executor.get("/path", TestPojo.class); + configServerApi.get("/path", TestPojo.class); assertLogStringContainsGETForAHost(); } |