Skip to content

Fixes default chunk size for Azure repositories #22577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 12, 2017
Merged

Fixes default chunk size for Azure repositories #22577

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eebf815
Fixes default chunk size for Azure repositories
Jan 12, 2017
41ec9bb
removes unneeded assertion
Jan 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...sitory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.unit.TimeValue;

Expand All @@ -42,6 +43,9 @@
*/
public interface AzureStorageService {

ByteSizeValue MIN_CHUNK_SIZE = new ByteSizeValue(1, ByteSizeUnit.BYTES);
ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(64, ByteSizeUnit.MB);

final class Storage {
public static final String PREFIX = "cloud.azure.storage.";

Expand All @@ -58,7 +62,7 @@ final class Storage {
public static final Setting<String> LOCATION_MODE_SETTING =
Setting.simpleString("repositories.azure.location_mode", Property.NodeScope);
public static final Setting<ByteSizeValue> CHUNK_SIZE_SETTING =
Setting.byteSizeSetting("repositories.azure.chunk_size", new ByteSizeValue(-1), Property.NodeScope);
Setting.byteSizeSetting("repositories.azure.chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope);
public static final Setting<Boolean> COMPRESS_SETTING =
Setting.boolSetting("repositories.azure.compress", false, Property.NodeScope);
}
Expand Down
18 changes: 4 additions & 14 deletions .../repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,14 @@
import org.elasticsearch.common.blobstore.BlobStore;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.repositories.RepositoryVerificationException;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
import org.elasticsearch.snapshots.SnapshotCreationException;

import static org.elasticsearch.cloud.azure.storage.AzureStorageSettings.getEffectiveSetting;
import static org.elasticsearch.cloud.azure.storage.AzureStorageService.MAX_CHUNK_SIZE;
import static org.elasticsearch.cloud.azure.storage.AzureStorageService.MIN_CHUNK_SIZE;
import static org.elasticsearch.cloud.azure.storage.AzureStorageSettings.getValue;

/**
Expand All @@ -64,8 +63,6 @@
*/
public class AzureRepository extends BlobStoreRepository {

private static final ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(64, ByteSizeUnit.MB);

public static final String TYPE = "azure";

public static final class Repository {
Expand All @@ -75,7 +72,7 @@ public static final class Repository {
public static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path", Property.NodeScope);
public static final Setting<String> LOCATION_MODE_SETTING = Setting.simpleString("location_mode", Property.NodeScope);
public static final Setting<ByteSizeValue> CHUNK_SIZE_SETTING =
Setting.byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, Property.NodeScope);
Setting.byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope);
public static final Setting<Boolean> COMPRESS_SETTING = Setting.boolSetting("compress", false, Property.NodeScope);
}

Expand All @@ -92,14 +89,7 @@ public AzureRepository(RepositoryMetaData metadata, Environment environment,

blobStore = new AzureBlobStore(metadata, environment.settings(), storageService);
String container = getValue(metadata.settings(), settings, Repository.CONTAINER_SETTING, Storage.CONTAINER_SETTING);
ByteSizeValue configuredChunkSize = getValue(metadata.settings(), settings, Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
if (configuredChunkSize.getMb() > MAX_CHUNK_SIZE.getMb()) {
Setting<ByteSizeValue> setting = getEffectiveSetting(metadata.settings(), Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
throw new SettingsException("[" + setting.getKey() + "] must not exceed [" + MAX_CHUNK_SIZE + "] but is set to [" + configuredChunkSize + "].");
} else {
this.chunkSize = configuredChunkSize;
}

this.chunkSize = getValue(metadata.settings(), settings, Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
this.compress = getValue(metadata.settings(), settings, Repository.COMPRESS_SETTING, Storage.COMPRESS_SETTING);
String modeStr = getValue(metadata.settings(), settings, Repository.LOCATION_MODE_SETTING, Storage.LOCATION_MODE_SETTING);
Boolean forcedReadonly = metadata.settings().getAsBoolean("readonly", null);
Expand Down
38 changes: 31 additions & 7 deletions ...zure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,21 @@

import com.microsoft.azure.storage.LocationMode;
import com.microsoft.azure.storage.StorageException;
import com.microsoft.azure.storage.blob.CloudBlobClient;
import org.elasticsearch.cloud.azure.storage.AzureStorageService;
import org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl;
import org.elasticsearch.cloud.azure.storage.AzureStorageSettings;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;

import static org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl.blobNameFromUri;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;

public class AzureRepositorySettingsTests extends ESTestCase {

Expand Down Expand Up @@ -103,4 +100,31 @@ public void testReadonlyWithPrimaryAndSecondaryOnlyAndReadonlyOff() throws Stora
.put("readonly", false)
.build()).isReadOnly(), is(false));
}

public void testChunkSize() throws StorageException, IOException, URISyntaxException {
// default chunk size
AzureRepository azureRepository = azureRepository(Settings.EMPTY);
assertEquals(AzureStorageService.MAX_CHUNK_SIZE, azureRepository.chunkSize());
assertThat(azureRepository.chunkSize().getBytes(), greaterThanOrEqualTo(0L));
Copy link
Member

@jasontedor jasontedor Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the value of this assertion given the assertion on the previous line?

Copy link
Author

@abeyad abeyad Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is sort of left over as the assertion is wrong (the chunk size should always be > 0, not >= 0). I'll remove this and the one below


// chunk size in settings
int size = randomIntBetween(1, 64);
azureRepository = azureRepository(Settings.builder().put("chunk_size", size + "mb").build());
assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), azureRepository.chunkSize());

// zero bytes is not allowed
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
azureRepository(Settings.builder().put("chunk_size", "0").build()));
assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());

// negative bytes not allowed
e = expectThrows(IllegalArgumentException.class, () ->
azureRepository(Settings.builder().put("chunk_size", "-1").build()));
assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());

// greater than max chunk size not allowed
e = expectThrows(IllegalArgumentException.class, () ->
azureRepository(Settings.builder().put("chunk_size", "65mb").build()));
assertEquals("Failed to parse value [65mb] for setting [chunk_size] must be <= 64mb", e.getMessage());
}
}
18 changes: 11 additions & 7 deletions ...ry-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@

public class GoogleCloudStorageRepository extends BlobStoreRepository {

// 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);

public static final String TYPE = "gcs";

public static final TimeValue NO_TIMEOUT = timeValueMillis(-1);
Expand All @@ -57,7 +61,7 @@ public class GoogleCloudStorageRepository extends BlobStoreRepository {
public static final Setting<Boolean> COMPRESS =
boolSetting("compress", false, Property.NodeScope, Property.Dynamic);
public static final Setting<ByteSizeValue> CHUNK_SIZE =
byteSizeSetting("chunk_size", new ByteSizeValue(100, ByteSizeUnit.MB), Property.NodeScope, Property.Dynamic);
byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope, Property.Dynamic);
public static final Setting<String> APPLICATION_NAME =
new Setting<>("application_name", GoogleCloudStoragePlugin.NAME, Function.identity(), Property.NodeScope, Property.Dynamic);
public static final Setting<String> SERVICE_ACCOUNT =
Expand All @@ -77,9 +81,9 @@ public GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment env
GoogleCloudStorageService storageService) throws Exception {
super(metadata, environment.settings(), namedXContentRegistry);

String bucket = get(BUCKET, metadata);
String application = get(APPLICATION_NAME, metadata);
String serviceAccount = get(SERVICE_ACCOUNT, metadata);
String bucket = getSetting(BUCKET, metadata);
String application = getSetting(APPLICATION_NAME, metadata);
String serviceAccount = getSetting(SERVICE_ACCOUNT, metadata);

String basePath = BASE_PATH.get(metadata.settings());
if (Strings.hasLength(basePath)) {
Expand All @@ -105,8 +109,8 @@ public GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment env
readTimeout = timeout;
}

this.compress = get(COMPRESS, metadata);
this.chunkSize = get(CHUNK_SIZE, metadata);
this.compress = getSetting(COMPRESS, metadata);
this.chunkSize = getSetting(CHUNK_SIZE, metadata);

logger.debug("using bucket [{}], base_path [{}], chunk_size [{}], compress [{}], application [{}]",
bucket, basePath, chunkSize, compress, application);
Expand Down Expand Up @@ -139,7 +143,7 @@ protected ByteSizeValue chunkSize() {
/**
* Get a given setting from the repository settings, throwing a {@link RepositoryException} if the setting does not exist or is empty.
*/
static <T> T get(Setting<T> setting, RepositoryMetaData metadata) {
static <T> T getSetting(Setting<T> setting, RepositoryMetaData metadata) {
T value = setting.get(metadata.settings());
if (value == null) {
throw new RepositoryException(metadata.name(), "Setting [" + setting.getKey() + "] is not defined for repository");
Expand Down
42 changes: 42 additions & 0 deletions ...t/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
package org.elasticsearch.repositories.gcs;

import com.google.api.services.storage.Storage;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.blobstore.gcs.MockHttpTransport;
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.plugin.repository.gcs.GoogleCloudStoragePlugin;
Expand All @@ -35,6 +37,7 @@
import java.util.concurrent.atomic.AtomicReference;

import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;

public class GoogleCloudStorageBlobStoreRepositoryTests extends ESBlobStoreRepositoryIntegTestCase {

Expand Down Expand Up @@ -80,4 +83,43 @@ public Storage createClient(String serviceAccount, String application, TimeValue
return storage.get();
}
}

public void testChunkSize() {
// default chunk size
RepositoryMetaData repositoryMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE, Settings.EMPTY);
ByteSizeValue chunkSize = GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repositoryMetaData);
assertEquals(GoogleCloudStorageRepository.MAX_CHUNK_SIZE, chunkSize);
assertThat(chunkSize.getBytes(), greaterThanOrEqualTo(0L));
Copy link
Member

@jasontedor jasontedor Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here?


// chunk size in settings
int size = randomIntBetween(1, 100);
repositoryMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
Settings.builder().put("chunk_size", size + "mb").build());
chunkSize = GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repositoryMetaData);
assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), chunkSize);

// zero bytes is not allowed
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
Settings.builder().put("chunk_size", "0").build());
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
});
assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());

// negative bytes not allowed
e = expectThrows(IllegalArgumentException.class, () -> {
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
Settings.builder().put("chunk_size", "-1").build());
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
});
assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());

// greater than max chunk size not allowed
e = expectThrows(IllegalArgumentException.class, () -> {
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
Settings.builder().put("chunk_size", "101mb").build());
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
});
assertEquals("Failed to parse value [101mb] for setting [chunk_size] must be <= 100mb", e.getMessage());
}
}