Skip to content

Commit f5fc163

Browse files
henningandersenhenningandersen
authored
Blob store compression fix (#39073)
Blob store compression was not enabled for some of the files in snapshots due to constructor accessing sub-class fields. Fixed to instead accept compress field as constructor param. Also fixed chunk size validation to work. Deprecated repositories.fs.compress setting as well to be able to unify in a future commit.
1 parent 18c5f93 commit f5fc163

File tree

8 files changed

+67
-64
lines changed

8 files changed

+67
-64
lines changed

‎modules/repository-url/src/main/java/org/elasticsearch/repositories/url/URLRepository.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public class URLRepository extends BlobStoreRepository {
8383
*/
8484
public URLRepository(RepositoryMetaData metadata, Environment environment,
8585
NamedXContentRegistry namedXContentRegistry) {
86-
super(metadata, environment.settings(), namedXContentRegistry);
86+
super(metadata, environment.settings(), false, namedXContentRegistry);
8787

8888
if (URL_SETTING.exists(metadata.settings()) == false && REPOSITORIES_URL_SETTING.exists(environment.settings()) == false) {
8989
throw new RepositoryException(metadata.name(), "missing url");

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

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
import com.microsoft.azure.storage.LocationMode;
2323
import com.microsoft.azure.storage.StorageException;
24-
2524
import org.apache.logging.log4j.LogManager;
2625
import org.apache.logging.log4j.Logger;
2726
import org.apache.logging.log4j.message.ParameterizedMessage;
@@ -82,16 +81,14 @@ public static final class Repository {
8281

8382
private final BlobPath basePath;
8483
private final ByteSizeValue chunkSize;
85-
private final boolean compress;
8684
private final Environment environment;
8785
private final AzureStorageService storageService;
8886
private final boolean readonly;
8987

9088
public AzureRepository(RepositoryMetaData metadata, Environment environment, NamedXContentRegistry namedXContentRegistry,
9189
AzureStorageService storageService) {
92-
super(metadata, environment.settings(), namedXContentRegistry);
90+
super(metadata, environment.settings(), Repository.COMPRESS_SETTING.get(metadata.settings()), namedXContentRegistry);
9391
this.chunkSize = Repository.CHUNK_SIZE_SETTING.get(metadata.settings());
94-
this.compress = Repository.COMPRESS_SETTING.get(metadata.settings());
9592
this.environment = environment;
9693
this.storageService = storageService;
9794

@@ -132,7 +129,7 @@ protected AzureBlobStore createBlobStore() throws URISyntaxException, StorageExc
132129

133130
logger.debug((org.apache.logging.log4j.util.Supplier<?>) () -> new ParameterizedMessage(
134131
"using container [{}], chunk_size [{}], compress [{}], base_path [{}]",
135-
blobStore, chunkSize, compress, basePath));
132+
blobStore, chunkSize, isCompress(), basePath));
136133
return blobStore;
137134
}
138135

@@ -141,14 +138,6 @@ protected BlobPath basePath() {
141138
return basePath;
142139
}
143140

144-
/**
145-
* {@inheritDoc}
146-
*/
147-
@Override
148-
protected boolean isCompress() {
149-
return compress;
150-
}
151-
152141
/**
153142
* {@inheritDoc}
154143
*/

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,14 @@ class GoogleCloudStorageRepository extends BlobStoreRepository {
6262
private final Settings settings;
6363
private final GoogleCloudStorageService storageService;
6464
private final BlobPath basePath;
65-
private final boolean compress;
6665
private final ByteSizeValue chunkSize;
6766
private final String bucket;
6867
private final String clientName;
6968

7069
GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment environment,
7170
NamedXContentRegistry namedXContentRegistry,
7271
GoogleCloudStorageService storageService) {
73-
super(metadata, environment.settings(), namedXContentRegistry);
72+
super(metadata, environment.settings(), getSetting(COMPRESS, metadata), namedXContentRegistry);
7473
this.settings = environment.settings();
7574
this.storageService = storageService;
7675

@@ -85,11 +84,10 @@ class GoogleCloudStorageRepository extends BlobStoreRepository {
8584
this.basePath = BlobPath.cleanPath();
8685
}
8786

88-
this.compress = getSetting(COMPRESS, metadata);
8987
this.chunkSize = getSetting(CHUNK_SIZE, metadata);
9088
this.bucket = getSetting(BUCKET, metadata);
9189
this.clientName = CLIENT_NAME.get(metadata.settings());
92-
logger.debug("using bucket [{}], base_path [{}], chunk_size [{}], compress [{}]", bucket, basePath, chunkSize, compress);
90+
logger.debug("using bucket [{}], base_path [{}], chunk_size [{}], compress [{}]", bucket, basePath, chunkSize, isCompress());
9391
}
9492

9593
@Override
@@ -102,11 +100,6 @@ protected BlobPath basePath() {
102100
return basePath;
103101
}
104102

105-
@Override
106-
protected boolean isCompress() {
107-
return compress;
108-
}
109-
110103
@Override
111104
protected ByteSizeValue chunkSize() {
112105
return chunkSize;

‎plugins/repository-hdfs/src/main/java/org/elasticsearch/repositories/hdfs/HdfsRepository.java‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ public final class HdfsRepository extends BlobStoreRepository {
5858

5959
private final Environment environment;
6060
private final ByteSizeValue chunkSize;
61-
private final boolean compress;
6261
private final BlobPath basePath = BlobPath.cleanPath();
6362
private final URI uri;
6463
private final String pathSetting;
@@ -69,11 +68,10 @@ public final class HdfsRepository extends BlobStoreRepository {
6968

7069
public HdfsRepository(RepositoryMetaData metadata, Environment environment,
7170
NamedXContentRegistry namedXContentRegistry) {
72-
super(metadata, environment.settings(), namedXContentRegistry);
71+
super(metadata, environment.settings(), metadata.settings().getAsBoolean("compress", false), namedXContentRegistry);
7372

7473
this.environment = environment;
7574
this.chunkSize = metadata.settings().getAsBytesSize("chunk_size", null);
76-
this.compress = metadata.settings().getAsBoolean("compress", false);
7775

7876
String uriSetting = getMetadata().settings().get("uri");
7977
if (Strings.hasText(uriSetting) == false) {
@@ -239,11 +237,6 @@ protected BlobPath basePath() {
239237
return basePath;
240238
}
241239

242-
@Override
243-
protected boolean isCompress() {
244-
return compress;
245-
}
246-
247240
@Override
248241
protected ByteSizeValue chunkSize() {
249242
return chunkSize;

‎plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@
1919

2020
package org.elasticsearch.repositories.s3;
2121

22-
import org.apache.logging.log4j.Logger;
2322
import org.apache.logging.log4j.LogManager;
23+
import org.apache.logging.log4j.Logger;
2424
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2525
import org.elasticsearch.common.Strings;
2626
import org.elasticsearch.common.blobstore.BlobPath;
@@ -155,8 +155,6 @@ class S3Repository extends BlobStoreRepository {
155155

156156
private final ByteSizeValue chunkSize;
157157

158-
private final boolean compress;
159-
160158
private final BlobPath basePath;
161159

162160
private final boolean serverSideEncryption;
@@ -174,7 +172,7 @@ class S3Repository extends BlobStoreRepository {
174172
final Settings settings,
175173
final NamedXContentRegistry namedXContentRegistry,
176174
final S3Service service) {
177-
super(metadata, settings, namedXContentRegistry);
175+
super(metadata, settings, COMPRESS_SETTING.get(metadata.settings()), namedXContentRegistry);
178176
this.service = service;
179177

180178
this.repositoryMetaData = metadata;
@@ -187,7 +185,6 @@ class S3Repository extends BlobStoreRepository {
187185

188186
this.bufferSize = BUFFER_SIZE_SETTING.get(metadata.settings());
189187
this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings());
190-
this.compress = COMPRESS_SETTING.get(metadata.settings());
191188

192189
// We make sure that chunkSize is bigger or equal than/to bufferSize
193190
if (this.chunkSize.getBytes() < bufferSize.getBytes()) {
@@ -245,11 +242,6 @@ protected BlobPath basePath() {
245242
return basePath;
246243
}
247244

248-
@Override
249-
protected boolean isCompress() {
250-
return compress;
251-
}
252-
253245
@Override
254246
protected ByteSizeValue chunkSize() {
255247
return chunkSize;

‎server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,8 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
195195

196196
private final Settings settings;
197197

198+
private final boolean compress;
199+
198200
private final RateLimiter snapshotRateLimiter;
199201

200202
private final RateLimiter restoreRateLimiter;
@@ -226,33 +228,37 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
226228
*
227229
* @param metadata The metadata for this repository including name and settings
228230
* @param settings Settings for the node this repository object is created on
231+
* @param compress true if metadata and snapshot files should be compressed
229232
*/
230-
protected BlobStoreRepository(RepositoryMetaData metadata, Settings settings, NamedXContentRegistry namedXContentRegistry) {
233+
protected BlobStoreRepository(RepositoryMetaData metadata, Settings settings, boolean compress,
234+
NamedXContentRegistry namedXContentRegistry) {
231235
this.settings = settings;
236+
this.compress = compress;
232237
this.metadata = metadata;
233238
this.namedXContentRegistry = namedXContentRegistry;
234239
snapshotRateLimiter = getRateLimiter(metadata.settings(), "max_snapshot_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB));
235240
restoreRateLimiter = getRateLimiter(metadata.settings(), "max_restore_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB));
236241
readOnly = metadata.settings().getAsBoolean("readonly", false);
237242

243+
238244
indexShardSnapshotFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_CODEC, SNAPSHOT_NAME_FORMAT,
239-
BlobStoreIndexShardSnapshot::fromXContent, namedXContentRegistry, isCompress());
245+
BlobStoreIndexShardSnapshot::fromXContent, namedXContentRegistry, compress);
240246
indexShardSnapshotsFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_INDEX_CODEC, SNAPSHOT_INDEX_NAME_FORMAT,
241-
BlobStoreIndexShardSnapshots::fromXContent, namedXContentRegistry, isCompress());
242-
ByteSizeValue chunkSize = chunkSize();
243-
if (chunkSize != null && chunkSize.getBytes() <= 0) {
244-
throw new IllegalArgumentException("the chunk size cannot be negative: [" + chunkSize + "]");
245-
}
247+
BlobStoreIndexShardSnapshots::fromXContent, namedXContentRegistry, compress);
246248
}
247249

248250
@Override
249251
protected void doStart() {
252+
ByteSizeValue chunkSize = chunkSize();
253+
if (chunkSize != null && chunkSize.getBytes() <= 0) {
254+
throw new IllegalArgumentException("the chunk size cannot be negative: [" + chunkSize + "]");
255+
}
250256
globalMetaDataFormat = new ChecksumBlobStoreFormat<>(METADATA_CODEC, METADATA_NAME_FORMAT,
251-
MetaData::fromXContent, namedXContentRegistry, isCompress());
257+
MetaData::fromXContent, namedXContentRegistry, compress);
252258
indexMetaDataFormat = new ChecksumBlobStoreFormat<>(INDEX_METADATA_CODEC, METADATA_NAME_FORMAT,
253-
IndexMetaData::fromXContent, namedXContentRegistry, isCompress());
259+
IndexMetaData::fromXContent, namedXContentRegistry, compress);
254260
snapshotFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_CODEC, SNAPSHOT_NAME_FORMAT,
255-
SnapshotInfo::fromXContentInternal, namedXContentRegistry, isCompress());
261+
SnapshotInfo::fromXContentInternal, namedXContentRegistry, compress);
256262
}
257263

258264
@Override
@@ -347,8 +353,8 @@ protected BlobStore blobStore() {
347353
*
348354
* @return true if compression is needed
349355
*/
350-
protected boolean isCompress() {
351-
return false;
356+
protected final boolean isCompress() {
357+
return compress;
352358
}
353359

354360
/**

‎server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -63,21 +63,19 @@ public class FsRepository extends BlobStoreRepository {
6363
new ByteSizeValue(Long.MAX_VALUE), new ByteSizeValue(5), new ByteSizeValue(Long.MAX_VALUE), Property.NodeScope);
6464
public static final Setting<Boolean> COMPRESS_SETTING = Setting.boolSetting("compress", false, Property.NodeScope);
6565
public static final Setting<Boolean> REPOSITORIES_COMPRESS_SETTING =
66-
Setting.boolSetting("repositories.fs.compress", false, Property.NodeScope);
66+
Setting.boolSetting("repositories.fs.compress", false, Property.NodeScope, Property.Deprecated);
6767
private final Environment environment;
6868

6969
private ByteSizeValue chunkSize;
7070

7171
private final BlobPath basePath;
7272

73-
private boolean compress;
74-
7573
/**
7674
* Constructs a shared file system repository.
7775
*/
7876
public FsRepository(RepositoryMetaData metadata, Environment environment,
7977
NamedXContentRegistry namedXContentRegistry) {
80-
super(metadata, environment.settings(), namedXContentRegistry);
78+
super(metadata, environment.settings(), calculateCompress(metadata, environment), namedXContentRegistry);
8179
this.environment = environment;
8280
String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings());
8381
if (location.isEmpty()) {
@@ -105,23 +103,21 @@ public FsRepository(RepositoryMetaData metadata, Environment environment,
105103
} else {
106104
this.chunkSize = REPOSITORIES_CHUNK_SIZE_SETTING.get(environment.settings());
107105
}
108-
this.compress = COMPRESS_SETTING.exists(metadata.settings())
109-
? COMPRESS_SETTING.get(metadata.settings()) : REPOSITORIES_COMPRESS_SETTING.get(environment.settings());
110106
this.basePath = BlobPath.cleanPath();
111107
}
112108

109+
private static boolean calculateCompress(RepositoryMetaData metadata, Environment environment) {
110+
return COMPRESS_SETTING.exists(metadata.settings())
111+
? COMPRESS_SETTING.get(metadata.settings()) : REPOSITORIES_COMPRESS_SETTING.get(environment.settings());
112+
}
113+
113114
@Override
114115
protected BlobStore createBlobStore() throws Exception {
115116
final String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings());
116117
final Path locationFile = environment.resolveRepoFile(location);
117118
return new FsBlobStore(environment.settings(), locationFile);
118119
}
119120

120-
@Override
121-
protected boolean isCompress() {
122-
return compress;
123-
}
124-
125121
@Override
126122
protected ByteSizeValue chunkSize() {
127123
return chunkSize;

‎server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,10 @@
2222
import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
2323
import org.elasticsearch.action.support.master.AcknowledgedResponse;
2424
import org.elasticsearch.client.Client;
25+
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2526
import org.elasticsearch.common.UUIDs;
2627
import org.elasticsearch.common.settings.Settings;
28+
import org.elasticsearch.common.unit.ByteSizeUnit;
2729
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
2830
import org.elasticsearch.env.Environment;
2931
import org.elasticsearch.plugins.Plugin;
@@ -232,6 +234,38 @@ public void testIncompatibleSnapshotsBlobExists() throws Exception {
232234
assertEquals(0, repository.getRepositoryData().getIncompatibleSnapshotIds().size());
233235
}
234236

237+
public void testBadChunksize() throws Exception {
238+
final Client client = client();
239+
final Path location = ESIntegTestCase.randomRepoPath(node().settings());
240+
final String repositoryName = "test-repo";
241+
242+
expectThrows(RepositoryException.class, () ->
243+
client.admin().cluster().preparePutRepository(repositoryName)
244+
.setType(REPO_TYPE)
245+
.setSettings(Settings.builder().put(node().settings())
246+
.put("location", location)
247+
.put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES))
248+
.get());
249+
}
250+
251+
public void testFsRepositoryCompressDeprecated() {
252+
final Path location = ESIntegTestCase.randomRepoPath(node().settings());
253+
final Settings settings = Settings.builder().put(node().settings()).put("location", location).build();
254+
final RepositoryMetaData metaData = new RepositoryMetaData("test-repo", REPO_TYPE, settings);
255+
256+
Settings useCompressSettings = Settings.builder()
257+
.put(node().getEnvironment().settings())
258+
.put(FsRepository.REPOSITORIES_COMPRESS_SETTING.getKey(), true)
259+
.build();
260+
Environment useCompressEnvironment =
261+
new Environment(useCompressSettings, node().getEnvironment().configFile());
262+
263+
new FsRepository(metaData, useCompressEnvironment, null);
264+
265+
assertWarnings("[repositories.fs.compress] setting was deprecated in Elasticsearch and will be removed in a future release!" +
266+
" See the breaking changes documentation for the next major version.");
267+
}
268+
235269
private BlobStoreRepository setupRepo() {
236270
final Client client = client();
237271
final Path location = ESIntegTestCase.randomRepoPath(node().settings());

0 commit comments

Comments
 (0)