Skip to content

Commit

Permalink
Fix the mishandled backport 873d380 (#30638)
Browse files Browse the repository at this point in the history 
Closes #30738
  • Loading branch information
@albertzaharovits
albertzaharovits committed May 16, 2018
1 parent 75ecf58 commit 0aafb06
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 16 deletions.
46 changes: 45 additions & 1 deletion ...ry-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<String> BUCKET =
simpleString("bucket", Property.NodeScope, Property.Dynamic);
static final Setting<String> BASE_PATH =
Expand All @@ -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<String> CLIENT_NAME = new Setting<>("client", "default", Function.identity());

@Deprecated
static final Setting<String> APPLICATION_NAME =
new Setting<>("application_name", "", Function.identity(), Property.NodeScope, Property.Dynamic);

@Deprecated
static final Setting<TimeValue> HTTP_READ_TIMEOUT =
timeSetting("http.read_timeout", NO_TIMEOUT, Property.NodeScope, Property.Dynamic);

@Deprecated
static final Setting<TimeValue> 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;
Expand Down Expand Up @@ -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);
}

Expand Down
18 changes: 13 additions & 5 deletions ...itory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -55,9 +56,15 @@ public GoogleCloudStorageService(final Environment environment, final Map<String
* Creates a client that can be used to manage Google Cloud Storage objects.
*
* @param clientName name of client settings to use, including secure settings
* @param application deprecated application name setting overriding client settings
* @param connectTimeout deprecated connect timeout setting overriding client settings
* @param readTimeout deprecated read timeout setting overriding client settings
* @return a Client instance that can be used to manage Storage objects
*/
public Storage createClient(final String clientName) throws Exception {
public Storage createClient(final String clientName,
@Nullable final String application,
@Nullable final TimeValue connectTimeout,
@Nullable final TimeValue readTimeout) throws Exception {

final GoogleCloudStorageClientSettings clientSettings = clientsSettings.get(clientName);
if (clientSettings == null) {
Expand All @@ -66,16 +73,17 @@ public Storage createClient(final String clientName) throws Exception {
}
final HttpTransport httpTransport = createHttpTransport(clientSettings.getHost());
final HttpTransportOptions httpTransportOptions = HttpTransportOptions.newBuilder()
.setConnectTimeout(toTimeout(clientSettings.getConnectTimeout()))
.setReadTimeout(toTimeout(clientSettings.getReadTimeout()))
.setConnectTimeout(connectTimeout != null ? toTimeout(connectTimeout) : toTimeout(clientSettings.getConnectTimeout()))
.setReadTimeout(readTimeout != null ? toTimeout(readTimeout) : toTimeout(clientSettings.getReadTimeout()))
.setHttpTransportFactory(() -> httpTransport)
.build();
final StorageOptions.Builder storageOptionsBuilder = StorageOptions.newBuilder()
.setTransportOptions(httpTransportOptions)
.setHeaderProvider(() -> {
final MapBuilder<String, String> 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();
});
Expand Down
6 changes: 5 additions & 1 deletion ...t/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepositoryDeprecationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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")
Expand All @@ -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<>());
}
});
Expand Down
33 changes: 26 additions & 7 deletions ...-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down

0 comments on commit 0aafb06

Please sign in to comment.