Skip to content

Commit 784fe34

Browse files
author
Ali Beyad
committed
Fixes default chunk size for Azure repositories (#22577)
Before, the default chunk size for Azure repositories was -1 bytes, which meant that if the chunk_size was not set on the Azure repository, nor as a node setting, then no data files would get written as part of the snapshot (because the BlobStoreRepository's PartSliceStream does not know how to process negative chunk sizes). This commit fixes the default chunk size for Azure repositories to be the same as the maximum chunk size. This commit also adds tests for both the Azure and Google Cloud repositories to ensure only valid chunk sizes can be set. Closes #22513
1 parent 9176d47 commit 784fe34

File tree

5 files changed

+88
-29
lines changed

5 files changed

+88
-29
lines changed

‎plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageService.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.elasticsearch.common.settings.Setting;
2727
import org.elasticsearch.common.settings.Setting.Property;
2828
import org.elasticsearch.common.settings.Settings;
29+
import org.elasticsearch.common.unit.ByteSizeUnit;
2930
import org.elasticsearch.common.unit.ByteSizeValue;
3031
import org.elasticsearch.common.unit.TimeValue;
3132

@@ -42,6 +43,9 @@
4243
*/
4344
public interface AzureStorageService {
4445

46+
ByteSizeValue MIN_CHUNK_SIZE = new ByteSizeValue(1, ByteSizeUnit.BYTES);
47+
ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(64, ByteSizeUnit.MB);
48+
4549
final class Storage {
4650
public static final String PREFIX = "cloud.azure.storage.";
4751

@@ -58,7 +62,7 @@ final class Storage {
5862
public static final Setting<String> LOCATION_MODE_SETTING =
5963
Setting.simpleString("repositories.azure.location_mode", Property.NodeScope);
6064
public static final Setting<ByteSizeValue> CHUNK_SIZE_SETTING =
61-
Setting.byteSizeSetting("repositories.azure.chunk_size", new ByteSizeValue(-1), Property.NodeScope);
65+
Setting.byteSizeSetting("repositories.azure.chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope);
6266
public static final Setting<Boolean> COMPRESS_SETTING =
6367
Setting.boolSetting("repositories.azure.compress", false, Property.NodeScope);
6468
}

‎plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepository.java‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,14 @@
4040
import org.elasticsearch.common.blobstore.BlobStore;
4141
import org.elasticsearch.common.settings.Setting;
4242
import org.elasticsearch.common.settings.Setting.Property;
43-
import org.elasticsearch.common.settings.SettingsException;
44-
import org.elasticsearch.common.unit.ByteSizeUnit;
4543
import org.elasticsearch.common.unit.ByteSizeValue;
4644
import org.elasticsearch.env.Environment;
4745
import org.elasticsearch.repositories.RepositoryVerificationException;
4846
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
4947
import org.elasticsearch.snapshots.SnapshotCreationException;
5048

51-
import static org.elasticsearch.cloud.azure.storage.AzureStorageSettings.getEffectiveSetting;
49+
import static org.elasticsearch.cloud.azure.storage.AzureStorageService.MAX_CHUNK_SIZE;
50+
import static org.elasticsearch.cloud.azure.storage.AzureStorageService.MIN_CHUNK_SIZE;
5251
import static org.elasticsearch.cloud.azure.storage.AzureStorageSettings.getValue;
5352

5453
/**
@@ -64,8 +63,6 @@
6463
*/
6564
public class AzureRepository extends BlobStoreRepository {
6665

67-
private static final ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(64, ByteSizeUnit.MB);
68-
6966
public static final String TYPE = "azure";
7067

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

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

9390
blobStore = new AzureBlobStore(metadata, environment.settings(), storageService);
9491
String container = getValue(metadata.settings(), settings, Repository.CONTAINER_SETTING, Storage.CONTAINER_SETTING);
95-
ByteSizeValue configuredChunkSize = getValue(metadata.settings(), settings, Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
96-
if (configuredChunkSize.getMb() > MAX_CHUNK_SIZE.getMb()) {
97-
Setting<ByteSizeValue> setting = getEffectiveSetting(metadata.settings(), Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
98-
throw new SettingsException("[" + setting.getKey() + "] must not exceed [" + MAX_CHUNK_SIZE + "] but is set to [" + configuredChunkSize + "].");
99-
} else {
100-
this.chunkSize = configuredChunkSize;
101-
}
102-
92+
this.chunkSize = getValue(metadata.settings(), settings, Repository.CHUNK_SIZE_SETTING, Storage.CHUNK_SIZE_SETTING);
10393
this.compress = getValue(metadata.settings(), settings, Repository.COMPRESS_SETTING, Storage.COMPRESS_SETTING);
10494
String modeStr = getValue(metadata.settings(), settings, Repository.LOCATION_MODE_SETTING, Storage.LOCATION_MODE_SETTING);
10595
Boolean forcedReadonly = metadata.settings().getAsBoolean("readonly", null);

‎plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,24 +21,19 @@
2121

2222
import com.microsoft.azure.storage.LocationMode;
2323
import com.microsoft.azure.storage.StorageException;
24-
import com.microsoft.azure.storage.blob.CloudBlobClient;
2524
import org.elasticsearch.cloud.azure.storage.AzureStorageService;
26-
import org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl;
27-
import org.elasticsearch.cloud.azure.storage.AzureStorageSettings;
2825
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2926
import org.elasticsearch.common.settings.Settings;
27+
import org.elasticsearch.common.unit.ByteSizeUnit;
28+
import org.elasticsearch.common.unit.ByteSizeValue;
3029
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
3130
import org.elasticsearch.env.Environment;
32-
import org.elasticsearch.env.NodeEnvironment;
3331
import org.elasticsearch.test.ESTestCase;
3432

3533
import java.io.IOException;
36-
import java.net.URI;
3734
import java.net.URISyntaxException;
3835

39-
import static org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl.blobNameFromUri;
4036
import static org.hamcrest.Matchers.is;
41-
import static org.hamcrest.Matchers.nullValue;
4237

4338
public class AzureRepositorySettingsTests extends ESTestCase {
4439

@@ -103,4 +98,30 @@ public void testReadonlyWithPrimaryAndSecondaryOnlyAndReadonlyOff() throws Stora
10398
.put("readonly", false)
10499
.build()).isReadOnly(), is(false));
105100
}
101+
102+
public void testChunkSize() throws StorageException, IOException, URISyntaxException {
103+
// default chunk size
104+
AzureRepository azureRepository = azureRepository(Settings.EMPTY);
105+
assertEquals(AzureStorageService.MAX_CHUNK_SIZE, azureRepository.chunkSize());
106+
107+
// chunk size in settings
108+
int size = randomIntBetween(1, 64);
109+
azureRepository = azureRepository(Settings.builder().put("chunk_size", size + "mb").build());
110+
assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), azureRepository.chunkSize());
111+
112+
// zero bytes is not allowed
113+
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
114+
azureRepository(Settings.builder().put("chunk_size", "0").build()));
115+
assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());
116+
117+
// negative bytes not allowed
118+
e = expectThrows(IllegalArgumentException.class, () ->
119+
azureRepository(Settings.builder().put("chunk_size", "-1").build()));
120+
assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());
121+
122+
// greater than max chunk size not allowed
123+
e = expectThrows(IllegalArgumentException.class, () ->
124+
azureRepository(Settings.builder().put("chunk_size", "65mb").build()));
125+
assertEquals("Failed to parse value [65mb] for setting [chunk_size] must be <= 64mb", e.getMessage());
126+
}
106127
}

‎plugins/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageRepository.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@
4646

4747
public class GoogleCloudStorageRepository extends BlobStoreRepository {
4848

49+
// package private for testing
50+
static final ByteSizeValue MIN_CHUNK_SIZE = new ByteSizeValue(1, ByteSizeUnit.BYTES);
51+
static final ByteSizeValue MAX_CHUNK_SIZE = new ByteSizeValue(100, ByteSizeUnit.MB);
52+
4953
public static final String TYPE = "gcs";
5054

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

80-
String bucket = get(BUCKET, metadata);
81-
String application = get(APPLICATION_NAME, metadata);
82-
String serviceAccount = get(SERVICE_ACCOUNT, metadata);
84+
String bucket = getSetting(BUCKET, metadata);
85+
String application = getSetting(APPLICATION_NAME, metadata);
86+
String serviceAccount = getSetting(SERVICE_ACCOUNT, metadata);
8387

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

108-
this.compress = get(COMPRESS, metadata);
109-
this.chunkSize = get(CHUNK_SIZE, metadata);
112+
this.compress = getSetting(COMPRESS, metadata);
113+
this.chunkSize = getSetting(CHUNK_SIZE, metadata);
110114

111115
logger.debug("using bucket [{}], base_path [{}], chunk_size [{}], compress [{}], application [{}]",
112116
bucket, basePath, chunkSize, compress, application);
@@ -139,7 +143,7 @@ protected ByteSizeValue chunkSize() {
139143
/**
140144
* Get a given setting from the repository settings, throwing a {@link RepositoryException} if the setting does not exist or is empty.
141145
*/
142-
static <T> T get(Setting<T> setting, RepositoryMetaData metadata) {
146+
static <T> T getSetting(Setting<T> setting, RepositoryMetaData metadata) {
143147
T value = setting.get(metadata.settings());
144148
if (value == null) {
145149
throw new RepositoryException(metadata.name(), "Setting [" + setting.getKey() + "] is not defined for repository");

‎plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@
2020
package org.elasticsearch.repositories.gcs;
2121

2222
import com.google.api.services.storage.Storage;
23+
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2324
import org.elasticsearch.common.blobstore.gcs.MockHttpTransport;
2425
import org.elasticsearch.common.settings.Settings;
2526
import org.elasticsearch.common.unit.ByteSizeUnit;
27+
import org.elasticsearch.common.unit.ByteSizeValue;
2628
import org.elasticsearch.common.unit.TimeValue;
2729
import org.elasticsearch.env.Environment;
2830
import org.elasticsearch.plugin.repository.gcs.GoogleCloudStoragePlugin;
@@ -80,4 +82,42 @@ public Storage createClient(String serviceAccount, String application, TimeValue
8082
return storage.get();
8183
}
8284
}
85+
86+
public void testChunkSize() {
87+
// default chunk size
88+
RepositoryMetaData repositoryMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE, Settings.EMPTY);
89+
ByteSizeValue chunkSize = GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repositoryMetaData);
90+
assertEquals(GoogleCloudStorageRepository.MAX_CHUNK_SIZE, chunkSize);
91+
92+
// chunk size in settings
93+
int size = randomIntBetween(1, 100);
94+
repositoryMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
95+
Settings.builder().put("chunk_size", size + "mb").build());
96+
chunkSize = GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repositoryMetaData);
97+
assertEquals(new ByteSizeValue(size, ByteSizeUnit.MB), chunkSize);
98+
99+
// zero bytes is not allowed
100+
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
101+
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
102+
Settings.builder().put("chunk_size", "0").build());
103+
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
104+
});
105+
assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());
106+
107+
// negative bytes not allowed
108+
e = expectThrows(IllegalArgumentException.class, () -> {
109+
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
110+
Settings.builder().put("chunk_size", "-1").build());
111+
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
112+
});
113+
assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());
114+
115+
// greater than max chunk size not allowed
116+
e = expectThrows(IllegalArgumentException.class, () -> {
117+
RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
118+
Settings.builder().put("chunk_size", "101mb").build());
119+
GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
120+
});
121+
assertEquals("Failed to parse value [101mb] for setting [chunk_size] must be <= 100mb", e.getMessage());
122+
}
83123
}

0 commit comments

Comments
 (0)