From 78e5405686b1914be531e532d3bbb33ef721a14a Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 22 Dec 2021 09:22:05 -0500 Subject: [PATCH 1/3] [plugin] repository-azure: add configuration settings for connect/write/response/read timeouts Signed-off-by: Andriy Redko --- .../azure/AzureRepositoryPlugin.java | 6 +- .../azure/AzureStorageService.java | 21 +++++ .../azure/AzureStorageSettings.java | 92 ++++++++++++++++++- .../azure/AzureStorageServiceTests.java | 57 ++++++++++++ 4 files changed, 171 insertions(+), 5 deletions(-) diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java index 1f829852dfcf9..938401fec356c 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java @@ -96,7 +96,11 @@ public List> getSettings() { AzureStorageSettings.MAX_RETRIES_SETTING, AzureStorageSettings.PROXY_TYPE_SETTING, AzureStorageSettings.PROXY_HOST_SETTING, - AzureStorageSettings.PROXY_PORT_SETTING + AzureStorageSettings.PROXY_PORT_SETTING, + AzureStorageSettings.CONNECTION_TIMEOUT_SETTING, + AzureStorageSettings.WRITE_TIMEOUT_SETTING, + AzureStorageSettings.READ_TIMEOUT_SETTING, + AzureStorageSettings.RESPONSE_TIMEOUT_SETTING ); } diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java index 0faa8283c372e..54e2901d515eb 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java @@ -64,6 +64,7 @@ import org.opensearch.common.settings.SettingsException; import org.opensearch.common.unit.ByteSizeUnit; import org.opensearch.common.unit.ByteSizeValue; +import org.opensearch.common.unit.TimeValue; import java.net.InetSocketAddress; import java.net.Proxy; @@ -178,6 +179,26 @@ private ClientState buildClient(AzureStorageSettings azureStorageSettings, BiCon clientBuilder.proxy(new ProxyOptions(type, (InetSocketAddress) proxy.address())); } + final TimeValue connectionTimeout = azureStorageSettings.getConnectionTimeout(); + if (connectionTimeout != null) { + clientBuilder.connectTimeout(Duration.ofMillis(connectionTimeout.millis())); + } + + final TimeValue writeTimeout = azureStorageSettings.getWriteTimeout(); + if (writeTimeout != null) { + clientBuilder.writeTimeout(Duration.ofMillis(writeTimeout.millis())); + } + + final TimeValue readTimeout = azureStorageSettings.getReadTimeout(); + if (readTimeout != null) { + clientBuilder.readTimeout(Duration.ofMillis(readTimeout.millis())); + } + + final TimeValue responseTimeout = azureStorageSettings.getResponseTimeout(); + if (responseTimeout != null) { + clientBuilder.responseTimeout(Duration.ofMillis(responseTimeout.millis())); + } + builder.httpClient(clientBuilder.build()); // We define a default exponential retry policy diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java index 3cc808a6fea57..30cf961f6c335 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java @@ -105,6 +105,42 @@ final class AzureStorageSettings { () -> KEY_SETTING ); + // See please NettyAsyncHttpClientBuilder#DEFAULT_CONNECT_TIMEOUT + public static final AffixSetting CONNECTION_TIMEOUT_SETTING = Setting.affixKeySetting( + AZURE_CLIENT_PREFIX_KEY, + "connection.timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(10), Property.NodeScope), + () -> ACCOUNT_SETTING, + () -> KEY_SETTING + ); + + // See please NettyAsyncHttpClientBuilder#DEFAULT_WRITE_TIMEOUT + public static final AffixSetting WRITE_TIMEOUT_SETTING = Setting.affixKeySetting( + AZURE_CLIENT_PREFIX_KEY, + "write.timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.NodeScope), + () -> ACCOUNT_SETTING, + () -> KEY_SETTING + ); + + // See please NettyAsyncHttpClientBuilder#DEFAULT_READ_TIMEOUT + public static final AffixSetting READ_TIMEOUT_SETTING = Setting.affixKeySetting( + AZURE_CLIENT_PREFIX_KEY, + "read.timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.NodeScope), + () -> ACCOUNT_SETTING, + () -> KEY_SETTING + ); + + // See please NettyAsyncHttpClientBuilder#DEFAULT_RESPONSE_TIMEOUT + public static final AffixSetting RESPONSE_TIMEOUT_SETTING = Setting.affixKeySetting( + AZURE_CLIENT_PREFIX_KEY, + "response.timeout", + (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(60), Property.NodeScope), + () -> ACCOUNT_SETTING, + () -> KEY_SETTING + ); + /** The type of the proxy to connect to azure through. Can be direct (no proxy, default), http or socks */ public static final AffixSetting PROXY_TYPE_SETTING = Setting.affixKeySetting( AZURE_CLIENT_PREFIX_KEY, @@ -142,6 +178,10 @@ final class AzureStorageSettings { private final int maxRetries; private final Proxy proxy; private final LocationMode locationMode; + private final TimeValue connectionTimeout; + private final TimeValue writeTimeout; + private final TimeValue readTimeout; + private final TimeValue responseTimeout; // copy-constructor private AzureStorageSettings( @@ -151,7 +191,11 @@ private AzureStorageSettings( TimeValue timeout, int maxRetries, Proxy proxy, - LocationMode locationMode + LocationMode locationMode, + TimeValue connectionTimeout, + TimeValue writeTimeout, + TimeValue readTimeout, + TimeValue responseTimeout ) { this.account = account; this.connectString = connectString; @@ -160,6 +204,10 @@ private AzureStorageSettings( this.maxRetries = maxRetries; this.proxy = proxy; this.locationMode = locationMode; + this.connectionTimeout = connectionTimeout; + this.writeTimeout = writeTimeout; + this.readTimeout = readTimeout; + this.responseTimeout = responseTimeout; } private AzureStorageSettings( @@ -171,7 +219,11 @@ private AzureStorageSettings( int maxRetries, Proxy.Type proxyType, String proxyHost, - Integer proxyPort + Integer proxyPort, + TimeValue connectionTimeout, + TimeValue writeTimeout, + TimeValue readTimeout, + TimeValue responseTimeout ) { this.account = account; this.connectString = buildConnectString(account, key, sasToken, endpointSuffix); @@ -197,6 +249,10 @@ private AzureStorageSettings( } } this.locationMode = LocationMode.PRIMARY_ONLY; + this.connectionTimeout = connectionTimeout; + this.writeTimeout = writeTimeout; + this.readTimeout = readTimeout; + this.responseTimeout = responseTimeout; } public String getEndpointSuffix() { @@ -245,6 +301,22 @@ public LocationMode getLocationMode() { return locationMode; } + public TimeValue getConnectionTimeout() { + return connectionTimeout; + } + + public TimeValue getWriteTimeout() { + return writeTimeout; + } + + public TimeValue getReadTimeout() { + return readTimeout; + } + + public TimeValue getResponseTimeout() { + return responseTimeout; + } + @Override public String toString() { final StringBuilder sb = new StringBuilder("AzureStorageSettings{"); @@ -254,6 +326,10 @@ public String toString() { sb.append(", maxRetries=").append(maxRetries); sb.append(", proxy=").append(proxy); sb.append(", locationMode='").append(locationMode).append('\''); + sb.append(", connectionTimeout='").append(connectionTimeout).append('\''); + sb.append(", writeTimeout='").append(writeTimeout).append('\''); + sb.append(", readTimeout='").append(readTimeout).append('\''); + sb.append(", responseTimeout='").append(responseTimeout).append('\''); sb.append('}'); return sb.toString(); } @@ -296,7 +372,11 @@ private static AzureStorageSettings getClientSettings(Settings settings, String getValue(settings, clientName, MAX_RETRIES_SETTING), getValue(settings, clientName, PROXY_TYPE_SETTING), getValue(settings, clientName, PROXY_HOST_SETTING), - getValue(settings, clientName, PROXY_PORT_SETTING) + getValue(settings, clientName, PROXY_PORT_SETTING), + getValue(settings, clientName, CONNECTION_TIMEOUT_SETTING), + getValue(settings, clientName, WRITE_TIMEOUT_SETTING), + getValue(settings, clientName, READ_TIMEOUT_SETTING), + getValue(settings, clientName, RESPONSE_TIMEOUT_SETTING) ); } } @@ -327,7 +407,11 @@ static Map overrideLocationMode( entry.getValue().timeout, entry.getValue().maxRetries, entry.getValue().proxy, - locationMode + locationMode, + entry.getValue().connectionTimeout, + entry.getValue().writeTimeout, + entry.getValue().readTimeout, + entry.getValue().responseTimeout ) ); } diff --git a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java index bb39f8815ad77..d2e36ba758043 100644 --- a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java +++ b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java @@ -43,6 +43,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsException; import org.opensearch.common.settings.SettingsModule; +import org.opensearch.common.unit.TimeValue; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; @@ -217,6 +218,62 @@ public void testGetSelectedClientDefaultTimeout() { assertThat(azureStorageService.getBlobRequestTimeout("azure3"), is(Duration.ofSeconds(30))); } + public void testClientDefaultConnectionTimeout() { + final Settings settings = Settings.builder() + .setSecureSettings(buildSecureSettings()) + .put("azure.client.azure3.connection.timeout", "25s") + .build(); + final AzureStorageService mock = storageServiceWithSettingsValidation(settings); + final TimeValue timeout = mock.storageSettings.get("azure3").getConnectionTimeout(); + + assertThat(timeout, notNullValue()); + assertThat(timeout, equalTo(TimeValue.timeValueSeconds(25))); + assertThat(mock.storageSettings.get("azure2").getConnectionTimeout(), notNullValue()); + assertThat(mock.storageSettings.get("azure2").getConnectionTimeout(), equalTo(TimeValue.timeValueSeconds(10))); + } + + public void testClientDefaultWriteTimeout() { + final Settings settings = Settings.builder() + .setSecureSettings(buildSecureSettings()) + .put("azure.client.azure3.write.timeout", "85s") + .build(); + final AzureStorageService mock = storageServiceWithSettingsValidation(settings); + final TimeValue timeout = mock.storageSettings.get("azure3").getWriteTimeout(); + + assertThat(timeout, notNullValue()); + assertThat(timeout, equalTo(TimeValue.timeValueSeconds(85))); + assertThat(mock.storageSettings.get("azure2").getWriteTimeout(), notNullValue()); + assertThat(mock.storageSettings.get("azure2").getWriteTimeout(), equalTo(TimeValue.timeValueSeconds(60))); + } + + public void testClientDefaultReadTimeout() { + final Settings settings = Settings.builder() + .setSecureSettings(buildSecureSettings()) + .put("azure.client.azure3.read.timeout", "120s") + .build(); + final AzureStorageService mock = storageServiceWithSettingsValidation(settings); + final TimeValue timeout = mock.storageSettings.get("azure3").getReadTimeout(); + + assertThat(timeout, notNullValue()); + assertThat(timeout, equalTo(TimeValue.timeValueSeconds(120))); + assertThat(mock.storageSettings.get("azure2").getReadTimeout(), notNullValue()); + assertThat(mock.storageSettings.get("azure2").getReadTimeout(), equalTo(TimeValue.timeValueSeconds(60))); + } + + public void testClientDefaultResponseTimeout() { + final Settings settings = Settings.builder() + .setSecureSettings(buildSecureSettings()) + .put("azure.client.azure3.response.timeout", "1ms") + .build(); + final AzureStorageService mock = storageServiceWithSettingsValidation(settings); + final TimeValue timeout = mock.storageSettings.get("azure3").getResponseTimeout(); + + assertThat(timeout, notNullValue()); + assertThat(timeout, equalTo(TimeValue.timeValueMillis(1))); + assertThat(mock.storageSettings.get("azure2").getResponseTimeout(), notNullValue()); + assertThat(mock.storageSettings.get("azure2").getResponseTimeout(), equalTo(TimeValue.timeValueSeconds(60))); + } + public void testGetSelectedClientNoTimeout() { final AzureStorageService azureStorageService = storageServiceWithSettingsValidation(buildSettings()); assertThat(azureStorageService.getBlobRequestTimeout("azure1"), nullValue()); From 69d08a9863f5f5a343a623eb10a6a38d3324c566 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 22 Dec 2021 13:03:59 -0500 Subject: [PATCH 2/3] Addressing code review comments: renaming connectionXxx to connectXxx Signed-off-by: Andriy Redko --- .../azure/AzureRepositoryPlugin.java | 2 +- .../azure/AzureStorageService.java | 6 ++--- .../azure/AzureStorageSettings.java | 24 +++++++++---------- .../azure/AzureStorageServiceTests.java | 10 ++++---- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java index 938401fec356c..aa41941436171 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepositoryPlugin.java @@ -97,7 +97,7 @@ public List> getSettings() { AzureStorageSettings.PROXY_TYPE_SETTING, AzureStorageSettings.PROXY_HOST_SETTING, AzureStorageSettings.PROXY_PORT_SETTING, - AzureStorageSettings.CONNECTION_TIMEOUT_SETTING, + AzureStorageSettings.CONNECT_TIMEOUT_SETTING, AzureStorageSettings.WRITE_TIMEOUT_SETTING, AzureStorageSettings.READ_TIMEOUT_SETTING, AzureStorageSettings.RESPONSE_TIMEOUT_SETTING diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java index 54e2901d515eb..6cd3a149c6957 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java @@ -179,9 +179,9 @@ private ClientState buildClient(AzureStorageSettings azureStorageSettings, BiCon clientBuilder.proxy(new ProxyOptions(type, (InetSocketAddress) proxy.address())); } - final TimeValue connectionTimeout = azureStorageSettings.getConnectionTimeout(); - if (connectionTimeout != null) { - clientBuilder.connectTimeout(Duration.ofMillis(connectionTimeout.millis())); + final TimeValue connectTimeout = azureStorageSettings.getConnectTimeout(); + if (connectTimeout != null) { + clientBuilder.connectTimeout(Duration.ofMillis(connectTimeout.millis())); } final TimeValue writeTimeout = azureStorageSettings.getWriteTimeout(); diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java index 30cf961f6c335..57dfe8206d5a8 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java @@ -106,9 +106,9 @@ final class AzureStorageSettings { ); // See please NettyAsyncHttpClientBuilder#DEFAULT_CONNECT_TIMEOUT - public static final AffixSetting CONNECTION_TIMEOUT_SETTING = Setting.affixKeySetting( + public static final AffixSetting CONNECT_TIMEOUT_SETTING = Setting.affixKeySetting( AZURE_CLIENT_PREFIX_KEY, - "connection.timeout", + "connect.timeout", (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(10), Property.NodeScope), () -> ACCOUNT_SETTING, () -> KEY_SETTING @@ -178,7 +178,7 @@ final class AzureStorageSettings { private final int maxRetries; private final Proxy proxy; private final LocationMode locationMode; - private final TimeValue connectionTimeout; + private final TimeValue connectTimeout; private final TimeValue writeTimeout; private final TimeValue readTimeout; private final TimeValue responseTimeout; @@ -192,7 +192,7 @@ private AzureStorageSettings( int maxRetries, Proxy proxy, LocationMode locationMode, - TimeValue connectionTimeout, + TimeValue connectTimeout, TimeValue writeTimeout, TimeValue readTimeout, TimeValue responseTimeout @@ -204,7 +204,7 @@ private AzureStorageSettings( this.maxRetries = maxRetries; this.proxy = proxy; this.locationMode = locationMode; - this.connectionTimeout = connectionTimeout; + this.connectTimeout = connectTimeout; this.writeTimeout = writeTimeout; this.readTimeout = readTimeout; this.responseTimeout = responseTimeout; @@ -220,7 +220,7 @@ private AzureStorageSettings( Proxy.Type proxyType, String proxyHost, Integer proxyPort, - TimeValue connectionTimeout, + TimeValue connectTimeout, TimeValue writeTimeout, TimeValue readTimeout, TimeValue responseTimeout @@ -249,7 +249,7 @@ private AzureStorageSettings( } } this.locationMode = LocationMode.PRIMARY_ONLY; - this.connectionTimeout = connectionTimeout; + this.connectTimeout = connectTimeout; this.writeTimeout = writeTimeout; this.readTimeout = readTimeout; this.responseTimeout = responseTimeout; @@ -301,8 +301,8 @@ public LocationMode getLocationMode() { return locationMode; } - public TimeValue getConnectionTimeout() { - return connectionTimeout; + public TimeValue getConnectTimeout() { + return connectTimeout; } public TimeValue getWriteTimeout() { @@ -326,7 +326,7 @@ public String toString() { sb.append(", maxRetries=").append(maxRetries); sb.append(", proxy=").append(proxy); sb.append(", locationMode='").append(locationMode).append('\''); - sb.append(", connectionTimeout='").append(connectionTimeout).append('\''); + sb.append(", connectTimeout='").append(connectTimeout).append('\''); sb.append(", writeTimeout='").append(writeTimeout).append('\''); sb.append(", readTimeout='").append(readTimeout).append('\''); sb.append(", responseTimeout='").append(responseTimeout).append('\''); @@ -373,7 +373,7 @@ private static AzureStorageSettings getClientSettings(Settings settings, String getValue(settings, clientName, PROXY_TYPE_SETTING), getValue(settings, clientName, PROXY_HOST_SETTING), getValue(settings, clientName, PROXY_PORT_SETTING), - getValue(settings, clientName, CONNECTION_TIMEOUT_SETTING), + getValue(settings, clientName, CONNECT_TIMEOUT_SETTING), getValue(settings, clientName, WRITE_TIMEOUT_SETTING), getValue(settings, clientName, READ_TIMEOUT_SETTING), getValue(settings, clientName, RESPONSE_TIMEOUT_SETTING) @@ -408,7 +408,7 @@ static Map overrideLocationMode( entry.getValue().maxRetries, entry.getValue().proxy, locationMode, - entry.getValue().connectionTimeout, + entry.getValue().connectTimeout, entry.getValue().writeTimeout, entry.getValue().readTimeout, entry.getValue().responseTimeout diff --git a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java index d2e36ba758043..785ebef7307bc 100644 --- a/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java +++ b/plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureStorageServiceTests.java @@ -218,18 +218,18 @@ public void testGetSelectedClientDefaultTimeout() { assertThat(azureStorageService.getBlobRequestTimeout("azure3"), is(Duration.ofSeconds(30))); } - public void testClientDefaultConnectionTimeout() { + public void testClientDefaultConnectTimeout() { final Settings settings = Settings.builder() .setSecureSettings(buildSecureSettings()) - .put("azure.client.azure3.connection.timeout", "25s") + .put("azure.client.azure3.connect.timeout", "25s") .build(); final AzureStorageService mock = storageServiceWithSettingsValidation(settings); - final TimeValue timeout = mock.storageSettings.get("azure3").getConnectionTimeout(); + final TimeValue timeout = mock.storageSettings.get("azure3").getConnectTimeout(); assertThat(timeout, notNullValue()); assertThat(timeout, equalTo(TimeValue.timeValueSeconds(25))); - assertThat(mock.storageSettings.get("azure2").getConnectionTimeout(), notNullValue()); - assertThat(mock.storageSettings.get("azure2").getConnectionTimeout(), equalTo(TimeValue.timeValueSeconds(10))); + assertThat(mock.storageSettings.get("azure2").getConnectTimeout(), notNullValue()); + assertThat(mock.storageSettings.get("azure2").getConnectTimeout(), equalTo(TimeValue.timeValueSeconds(10))); } public void testClientDefaultWriteTimeout() { From d728932208d3ee68e8a7ee5c50bb63b5aba13fbb Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 22 Dec 2021 13:07:40 -0500 Subject: [PATCH 3/3] Addressing code review comments: adding timeout comment Signed-off-by: Andriy Redko --- .../org/opensearch/repositories/azure/AzureStorageSettings.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java index 57dfe8206d5a8..94ec553ab760e 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java @@ -97,6 +97,7 @@ final class AzureStorageSettings { () -> KEY_SETTING ); + // The overall operation timeout public static final AffixSetting TIMEOUT_SETTING = Setting.affixKeySetting( AZURE_CLIENT_PREFIX_KEY, "timeout",