From 0aafb060c640ae0bb230e69fed088e702ea2a2ec Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Wed, 16 May 2018 14:50:05 +0300 Subject: [PATCH] Fix the mishandled backport 873d380 (#30638) Closes #30738 --- .../gcs/GoogleCloudStorageRepository.java | 46 ++++++++++++++++++- .../gcs/GoogleCloudStorageService.java | 18 ++++++-- ...eCloudStorageBlobStoreRepositoryTests.java | 6 ++- ...loudStorageRepositoryDeprecationTests.java | 4 +- .../gcs/GoogleCloudStorageServiceTests.java | 33 ++++++++++--- 5 files changed, 91 insertions(+), 16 deletions(-) diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java index 422d7a308f260..d261d738e5eee 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java @@ -19,6 +19,7 @@ package org.elasticsearch.repositories.gcs; +import org.apache.logging.log4j.Logger; import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobPath; @@ -28,6 +29,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; +import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.repositories.RepositoryException; @@ -46,12 +48,17 @@ class GoogleCloudStorageRepository extends BlobStoreRepository { + private final Logger logger = ESLoggerFactory.getLogger(GoogleCloudStorageRepository.class); + private final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); + // package private for testing static final ByteSizeValue MIN_CHUNK_SIZE = new ByteSizeValue(1, ByteSizeUnit.BYTES); static final ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(100, ByteSizeUnit.MB); static final String TYPE = "gcs"; + static final TimeValue NO_TIMEOUT = timeValueMillis(-1); + static final Setting BUCKET = simpleString("bucket", Property.NodeScope, Property.Dynamic); static final Setting BASE_PATH = @@ -62,6 +69,18 @@ class GoogleCloudStorageRepository extends BlobStoreRepository { byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope, Property.Dynamic); static final Setting CLIENT_NAME = new Setting<>("client", "default", Function.identity()); + @Deprecated + static final Setting APPLICATION_NAME = + new Setting<>("application_name", "", Function.identity(), Property.NodeScope, Property.Dynamic); + + @Deprecated + static final Setting HTTP_READ_TIMEOUT = + timeSetting("http.read_timeout", NO_TIMEOUT, Property.NodeScope, Property.Dynamic); + + @Deprecated + static final Setting HTTP_CONNECT_TIMEOUT = + timeSetting("http.connect_timeout", NO_TIMEOUT, Property.NodeScope, Property.Dynamic); + private final ByteSizeValue chunkSize; private final boolean compress; private final BlobPath basePath; @@ -90,7 +109,32 @@ class GoogleCloudStorageRepository extends BlobStoreRepository { logger.debug("using bucket [{}], base_path [{}], chunk_size [{}], compress [{}]", bucket, basePath, chunkSize, compress); - Storage client = SocketAccess.doPrivilegedIOException(() -> storageService.createClient(clientName)); + String application = APPLICATION_NAME.get(metadata.settings()); + if (Strings.hasText(application)) { + deprecationLogger.deprecated("Setting [application_name] in repository settings is deprecated, " + + "it must be specified in the client settings instead"); + } + TimeValue connectTimeout = null; + TimeValue readTimeout = null; + + TimeValue timeout = HTTP_CONNECT_TIMEOUT.get(metadata.settings()); + if ((timeout != null) && (timeout.millis() != NO_TIMEOUT.millis())) { + deprecationLogger.deprecated("Setting [http.connect_timeout] in repository settings is deprecated, " + + "it must be specified in the client settings instead"); + connectTimeout = timeout; + } + timeout = HTTP_READ_TIMEOUT.get(metadata.settings()); + if ((timeout != null) && (timeout.millis() != NO_TIMEOUT.millis())) { + deprecationLogger.deprecated("Setting [http.read_timeout] in repository settings is deprecated, " + + "it must be specified in the client settings instead"); + readTimeout = timeout; + } + + TimeValue finalConnectTimeout = connectTimeout; + TimeValue finalReadTimeout = readTimeout; + + Storage client = SocketAccess.doPrivilegedIOException(() -> + storageService.createClient(clientName, application, finalConnectTimeout, finalReadTimeout)); this.blobStore = new GoogleCloudStorageBlobStore(settings, bucket, client); } diff --git a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java index 5a52fff463499..d3fa18ead0754 100644 --- a/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java +++ b/plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java @@ -28,6 +28,7 @@ import com.google.cloud.storage.Storage; import com.google.cloud.storage.StorageOptions; +import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.component.AbstractComponent; @@ -55,9 +56,15 @@ public GoogleCloudStorageService(final Environment environment, final Map httpTransport) .build(); final StorageOptions.Builder storageOptionsBuilder = StorageOptions.newBuilder() .setTransportOptions(httpTransportOptions) .setHeaderProvider(() -> { final MapBuilder mapBuilder = MapBuilder.newMapBuilder(); - if (Strings.hasLength(clientSettings.getApplicationName())) { - mapBuilder.put("user-agent", clientSettings.getApplicationName()); + final String applicationName = Strings.hasLength(application) ? application : clientSettings.getApplicationName(); + if (Strings.hasLength(applicationName)) { + mapBuilder.put("user-agent", applicationName); } return mapBuilder.immutableMap(); }); diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java index d02100f63cc41..6ed67c1a26947 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; +import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.env.Environment; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.repositories.blobstore.ESBlobStoreRepositoryIntegTestCase; @@ -85,7 +86,10 @@ public static class MockGoogleCloudStorageService extends GoogleCloudStorageServ } @Override - public Storage createClient(final String clientName) { + public Storage createClient(final String clientName, + final String application, + final TimeValue connectTimeout, + final TimeValue readTimeout) { return new MockStorage(BUCKET, blobs); } } diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepositoryDeprecationTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepositoryDeprecationTests.java index daf761a74dd43..e1f91eb1d31d3 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepositoryDeprecationTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepositoryDeprecationTests.java @@ -22,6 +22,7 @@ import com.google.cloud.storage.Storage; import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; @@ -31,7 +32,6 @@ public class GoogleCloudStorageRepositoryDeprecationTests extends ESTestCase { - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/30638") public void testDeprecatedSettings() throws Exception { final Settings repositorySettings = Settings.builder() .put("bucket", "test") @@ -46,7 +46,7 @@ public void testDeprecatedSettings() throws Exception { new GoogleCloudStorageRepository(repositoryMetaData, environment, NamedXContentRegistry.EMPTY, new GoogleCloudStorageService(environment, GoogleCloudStorageClientSettings.load(Settings.EMPTY)) { @Override - public Storage createClient(String clientName) throws Exception { + public Storage createClient(String clientName, String application, TimeValue connect, TimeValue read) throws Exception { return new MockStorage("test", new ConcurrentHashMap<>()); } }); diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java index a33ae90c549bc..4e87031a630b2 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java @@ -59,19 +59,38 @@ public void testClientInitializer() throws Exception { final GoogleCloudStorageClientSettings clientSettings = GoogleCloudStorageClientSettings.getClientSettings(settings, clientName); final GoogleCloudStorageService service = new GoogleCloudStorageService(environment, Collections.singletonMap(clientName, clientSettings)); - final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> service.createClient("another_client")); + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> service.createClient("another_client", null, null, null)); assertThat(e.getMessage(), Matchers.startsWith("Unknown client name")); assertSettingDeprecationsAndWarnings( new Setting[] { GoogleCloudStorageClientSettings.APPLICATION_NAME_SETTING.getConcreteSettingForNamespace(clientName) }); - final Storage storage = service.createClient(clientName); - assertThat(storage.getOptions().getApplicationName(), Matchers.containsString(applicationName)); + final String deprecatedApplicationName = randomBoolean() ? null : "deprecated_" + randomAlphaOfLength(4); + final TimeValue deprecatedConnectTimeout = randomBoolean() ? null : TimeValue.timeValueNanos(randomIntBetween(0, 2000000)); + final TimeValue deprecatedReadTimeout = randomBoolean() ? null : TimeValue.timeValueNanos(randomIntBetween(0, 2000000)); + final Storage storage = service.createClient(clientName, deprecatedApplicationName, deprecatedConnectTimeout, + deprecatedReadTimeout); + if (deprecatedApplicationName != null) { + assertThat(storage.getOptions().getApplicationName(), Matchers.containsString(deprecatedApplicationName)); + } else { + assertThat(storage.getOptions().getApplicationName(), Matchers.containsString(applicationName)); + } assertThat(storage.getOptions().getHost(), Matchers.is(hostName)); assertThat(storage.getOptions().getProjectId(), Matchers.is(projectIdName)); assertThat(storage.getOptions().getTransportOptions(), Matchers.instanceOf(HttpTransportOptions.class)); - assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getConnectTimeout(), - Matchers.is((int) connectTimeValue.millis())); - assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getReadTimeout(), - Matchers.is((int) readTimeValue.millis())); + if (deprecatedConnectTimeout != null) { + assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getConnectTimeout(), + Matchers.is((int) deprecatedConnectTimeout.millis())); + } else { + assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getConnectTimeout(), + Matchers.is((int) connectTimeValue.millis())); + } + if (deprecatedReadTimeout != null) { + assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getReadTimeout(), + Matchers.is((int) deprecatedReadTimeout.millis())); + } else { + assertThat(((HttpTransportOptions) storage.getOptions().getTransportOptions()).getReadTimeout(), + Matchers.is((int) readTimeValue.millis())); + } assertThat(storage.getOptions().getCredentials(), Matchers.nullValue(Credentials.class)); }