From 80ef34d7f0f33777e4c6b732004a2e84a9c343f3 Mon Sep 17 00:00:00 2001 From: Kris De Schutter Date: Wed, 30 Jan 2019 12:38:35 +0100 Subject: [PATCH 1/5] * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java#x4e06dc48702dac54:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#xaeb362a26c0fdaf1:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#x5c00888239d02f3a:1) --- .../blobstore/BlobStoreRepository.java | 2 +- .../repositories/fs/FsRepository.java | 32 +++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index c8cdf0d4e0308..a1002ac6b0b17 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -193,7 +193,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private static final String DATA_BLOB_PREFIX = "__"; - private final Settings settings; + protected final Settings settings; private final RateLimiter snapshotRateLimiter; diff --git a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java index 01c08fbce0044..cbff70fbc5b20 100644 --- a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java @@ -66,10 +66,12 @@ public class FsRepository extends BlobStoreRepository { Setting.boolSetting("repositories.fs.compress", false, Property.NodeScope); private final Environment environment; + private boolean chunkSizeInitialized = false; private ByteSizeValue chunkSize; private final BlobPath basePath; + private boolean compressInitialized = false; private boolean compress; /** @@ -100,13 +102,6 @@ public FsRepository(RepositoryMetaData metadata, Environment environment, } } - if (CHUNK_SIZE_SETTING.exists(metadata.settings())) { - this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings()); - } else { - this.chunkSize = REPOSITORIES_CHUNK_SIZE_SETTING.get(environment.settings()); - } - this.compress = COMPRESS_SETTING.exists(metadata.settings()) - ? COMPRESS_SETTING.get(metadata.settings()) : REPOSITORIES_COMPRESS_SETTING.get(environment.settings()); this.basePath = BlobPath.cleanPath(); } @@ -117,13 +112,36 @@ protected BlobStore createBlobStore() throws Exception { return new FsBlobStore(environment.settings(), locationFile); } + /* + * Note: this method gets called by the super constructor, so we can't rely on instance fields in this class having been initialized + * yet. + */ @Override protected boolean isCompress() { + if (!compressInitialized) { + this.compress = COMPRESS_SETTING.exists(metadata.settings()) ? COMPRESS_SETTING.get(metadata.settings()) + : REPOSITORIES_COMPRESS_SETTING.get(settings); + compressInitialized = true; + } + return compress; } + /* + * Note: this method gets called by the super constructor, so we can't rely on instance fields in this class having been initialized + * yet. + */ @Override protected ByteSizeValue chunkSize() { + if (!chunkSizeInitialized) { + if (CHUNK_SIZE_SETTING.exists(metadata.settings())) { + this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings()); + } else { + this.chunkSize = REPOSITORIES_CHUNK_SIZE_SETTING.get(settings); + } + chunkSizeInitialized = true; + } + return chunkSize; } From 9b68bfdf7ad31566c2c624f3bcacd41e80479259 Mon Sep 17 00:00:00 2001 From: Kris De Schutter Date: Wed, 6 Feb 2019 11:38:13 +0100 Subject: [PATCH 2/5] Move initialization from constructor to `doStart`. --- .../blobstore/BlobStoreRepository.java | 24 +++++++------- .../repositories/fs/FsRepository.java | 32 ++++--------------- 2 files changed, 19 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index a1002ac6b0b17..cde7a18d8c11c 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -193,7 +193,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private static final String DATA_BLOB_PREFIX = "__"; - protected final Settings settings; + private final Settings settings; private final RateLimiter snapshotRateLimiter; @@ -211,9 +211,9 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private final boolean readOnly; - private final ChecksumBlobStoreFormat indexShardSnapshotFormat; + private ChecksumBlobStoreFormat indexShardSnapshotFormat; - private final ChecksumBlobStoreFormat indexShardSnapshotsFormat; + private ChecksumBlobStoreFormat indexShardSnapshotsFormat; private final Object lock = new Object(); @@ -234,15 +234,6 @@ protected BlobStoreRepository(RepositoryMetaData metadata, Settings settings, Na snapshotRateLimiter = getRateLimiter(metadata.settings(), "max_snapshot_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB)); restoreRateLimiter = getRateLimiter(metadata.settings(), "max_restore_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB)); readOnly = metadata.settings().getAsBoolean("readonly", false); - - indexShardSnapshotFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_CODEC, SNAPSHOT_NAME_FORMAT, - BlobStoreIndexShardSnapshot::fromXContent, namedXContentRegistry, isCompress()); - indexShardSnapshotsFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_INDEX_CODEC, SNAPSHOT_INDEX_NAME_FORMAT, - BlobStoreIndexShardSnapshots::fromXContent, namedXContentRegistry, isCompress()); - ByteSizeValue chunkSize = chunkSize(); - if (chunkSize != null && chunkSize.getBytes() <= 0) { - throw new IllegalArgumentException("the chunk size cannot be negative: [" + chunkSize + "]"); - } } @Override @@ -253,6 +244,15 @@ protected void doStart() { IndexMetaData::fromXContent, namedXContentRegistry, isCompress()); snapshotFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_CODEC, SNAPSHOT_NAME_FORMAT, SnapshotInfo::fromXContentInternal, namedXContentRegistry, isCompress()); + + indexShardSnapshotFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_CODEC, SNAPSHOT_NAME_FORMAT, + BlobStoreIndexShardSnapshot::fromXContent, namedXContentRegistry, isCompress()); + indexShardSnapshotsFormat = new ChecksumBlobStoreFormat<>(SNAPSHOT_INDEX_CODEC, SNAPSHOT_INDEX_NAME_FORMAT, + BlobStoreIndexShardSnapshots::fromXContent, namedXContentRegistry, isCompress()); + ByteSizeValue chunkSize = chunkSize(); + if (chunkSize != null && chunkSize.getBytes() <= 0) { + throw new IllegalArgumentException("the chunk size cannot be negative: [" + chunkSize + "]"); + } } @Override diff --git a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java index cbff70fbc5b20..01c08fbce0044 100644 --- a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java @@ -66,12 +66,10 @@ public class FsRepository extends BlobStoreRepository { Setting.boolSetting("repositories.fs.compress", false, Property.NodeScope); private final Environment environment; - private boolean chunkSizeInitialized = false; private ByteSizeValue chunkSize; private final BlobPath basePath; - private boolean compressInitialized = false; private boolean compress; /** @@ -102,6 +100,13 @@ public FsRepository(RepositoryMetaData metadata, Environment environment, } } + if (CHUNK_SIZE_SETTING.exists(metadata.settings())) { + this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings()); + } else { + this.chunkSize = REPOSITORIES_CHUNK_SIZE_SETTING.get(environment.settings()); + } + this.compress = COMPRESS_SETTING.exists(metadata.settings()) + ? COMPRESS_SETTING.get(metadata.settings()) : REPOSITORIES_COMPRESS_SETTING.get(environment.settings()); this.basePath = BlobPath.cleanPath(); } @@ -112,36 +117,13 @@ protected BlobStore createBlobStore() throws Exception { return new FsBlobStore(environment.settings(), locationFile); } - /* - * Note: this method gets called by the super constructor, so we can't rely on instance fields in this class having been initialized - * yet. - */ @Override protected boolean isCompress() { - if (!compressInitialized) { - this.compress = COMPRESS_SETTING.exists(metadata.settings()) ? COMPRESS_SETTING.get(metadata.settings()) - : REPOSITORIES_COMPRESS_SETTING.get(settings); - compressInitialized = true; - } - return compress; } - /* - * Note: this method gets called by the super constructor, so we can't rely on instance fields in this class having been initialized - * yet. - */ @Override protected ByteSizeValue chunkSize() { - if (!chunkSizeInitialized) { - if (CHUNK_SIZE_SETTING.exists(metadata.settings())) { - this.chunkSize = CHUNK_SIZE_SETTING.get(metadata.settings()); - } else { - this.chunkSize = REPOSITORIES_CHUNK_SIZE_SETTING.get(settings); - } - chunkSizeInitialized = true; - } - return chunkSize; } From a2c2046ee199f94ae3d28c45a3e059b5886c9c1a Mon Sep 17 00:00:00 2001 From: Kris De Schutter Date: Wed, 6 Feb 2019 12:49:27 +0100 Subject: [PATCH 3/5] Test error on bad chunk size setting. --- .../blobstore/BlobStoreRepositoryTests.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java index 871e5071ec7b7..355bb9ab20afb 100644 --- a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.client.Client; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.plugins.Plugin; @@ -38,6 +39,7 @@ import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESSingleNodeTestCase; +import org.junit.Test; import java.io.IOException; import java.nio.file.Path; @@ -232,6 +234,20 @@ public void testIncompatibleSnapshotsBlobExists() throws Exception { assertEquals(0, repository.getRepositoryData().getIncompatibleSnapshotIds().size()); } + @Test(expected = RepositoryException.class) + public void testBadChunksize() throws Exception { + final Client client = client(); + final Path location = ESIntegTestCase.randomRepoPath(node().settings()); + final String repositoryName = "test-repo"; + + client.admin().cluster().preparePutRepository(repositoryName) + .setType(REPO_TYPE) + .setSettings(Settings.builder().put(node().settings()) + .put("location", location) + .put("chunk_size", randomIntBetween(Integer.MIN_VALUE, 4), ByteSizeUnit.BYTES)) + .get(); + } + private BlobStoreRepository setupRepo() { final Client client = client(); final Path location = ESIntegTestCase.randomRepoPath(node().settings()); From cb110f1acbbdca73299b6e6dd76a458ada2cf87e Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 6 Feb 2019 18:03:12 +0100 Subject: [PATCH 4/5] Fix compression of snapshot metadata Added test case ensuring that compress and chunkSize are not called in the BlobStoreRepository constructor, since this is unsafe. Plus fixed testBadChunks to only use illegal chunkSizes for verification. --- .../blobstore/BlobStoreRepositoryTests.java | 37 ++++++++++++++++++- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java index 355bb9ab20afb..1e55fa8af8846 100644 --- a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -22,9 +22,13 @@ import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.client.Client; +import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.UUIDs; +import org.elasticsearch.common.blobstore.BlobPath; +import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.settings.Settings; 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.plugins.Plugin; @@ -234,6 +238,35 @@ public void testIncompatibleSnapshotsBlobExists() throws Exception { assertEquals(0, repository.getRepositoryData().getIncompatibleSnapshotIds().size()); } + + public void testBlobStoreRepositoryConstructor() { + // simply check that compress and chunksize are not called (since they used to be) - they are not valid inside the constructor, + // call them in doStart() instead. + new BlobStoreRepository(new RepositoryMetaData("test", "test", Settings.EMPTY), Settings.EMPTY, null) { + @Override + protected boolean isCompress() { + fail("Not allowed to call isCompress in constructor"); + return false; + } + + @Override + protected ByteSizeValue chunkSize() { + fail("Not allowed to call chunkSize in constructor"); + return null; + } + + @Override + protected BlobStore createBlobStore() throws Exception { + throw new UnsupportedOperationException(); + } + + @Override + protected BlobPath basePath() { + throw new UnsupportedOperationException(); + } + }; + } + @Test(expected = RepositoryException.class) public void testBadChunksize() throws Exception { final Client client = client(); @@ -244,9 +277,9 @@ public void testBadChunksize() throws Exception { .setType(REPO_TYPE) .setSettings(Settings.builder().put(node().settings()) .put("location", location) - .put("chunk_size", randomIntBetween(Integer.MIN_VALUE, 4), ByteSizeUnit.BYTES)) + .put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES)) .get(); - } + } private BlobStoreRepository setupRepo() { final Client client = client(); From ec35d338c0dda524a57191b3ed0255954676f562 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 6 Feb 2019 18:43:27 +0100 Subject: [PATCH 5/5] Fix compression of snapshot metadata Fixed test case to not rely on Test annotation. --- .../blobstore/BlobStoreRepositoryTests.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java index 1e55fa8af8846..5f7a4afc78815 100644 --- a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -43,7 +43,6 @@ import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESSingleNodeTestCase; -import org.junit.Test; import java.io.IOException; import java.nio.file.Path; @@ -267,18 +266,18 @@ protected BlobPath basePath() { }; } - @Test(expected = RepositoryException.class) public void testBadChunksize() throws Exception { final Client client = client(); final Path location = ESIntegTestCase.randomRepoPath(node().settings()); final String repositoryName = "test-repo"; - client.admin().cluster().preparePutRepository(repositoryName) - .setType(REPO_TYPE) - .setSettings(Settings.builder().put(node().settings()) - .put("location", location) - .put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES)) - .get(); + expectThrows(RepositoryException.class, () -> + client.admin().cluster().preparePutRepository(repositoryName) + .setType(REPO_TYPE) + .setSettings(Settings.builder().put(node().settings()) + .put("location", location) + .put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES)) + .get()); } private BlobStoreRepository setupRepo() {