Skip to content
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

Remove node settings from blob store repositories #45991

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Remove node settings from blob store repositories #45991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion modules/repository-url/src/main/java/org/elasticsearch/repositories/url/URLRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public class URLRepository extends BlobStoreRepository {
*/
public URLRepository(RepositoryMetaData metadata, Environment environment,
NamedXContentRegistry namedXContentRegistry, ThreadPool threadPool) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, BlobPath.cleanPath());
super(metadata, namedXContentRegistry, threadPool, BlobPath.cleanPath());

if (URL_SETTING.exists(metadata.settings()) == false && REPOSITORIES_URL_SETTING.exists(environment.settings()) == false) {
throw new RepositoryException(metadata.name(), "missing url");
Expand Down
10 changes: 6 additions & 4 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 @@ -31,7 +31,6 @@
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.Environment;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
import org.elasticsearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -76,9 +75,12 @@ public static final class Repository {
private final AzureStorageService storageService;
private final boolean readonly;

public AzureRepository(RepositoryMetaData metadata, Environment environment, NamedXContentRegistry namedXContentRegistry,
AzureStorageService storageService, ThreadPool threadPool) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, buildBasePath(metadata));
public AzureRepository(
final RepositoryMetaData metadata,
final NamedXContentRegistry namedXContentRegistry,
final AzureStorageService storageService,
final ThreadPool threadPool) {
super(metadata, namedXContentRegistry, threadPool, buildBasePath(metadata));
this.chunkSize = Repository.CHUNK_SIZE_SETTING.get(metadata.settings());
this.storageService = storageService;

Expand Down
2 changes: 1 addition & 1 deletion ...itory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepositoryPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public AzureRepositoryPlugin(Settings settings) {
public Map<String, Repository.Factory> getRepositories(Environment env, NamedXContentRegistry namedXContentRegistry,
ThreadPool threadPool) {
return Collections.singletonMap(AzureRepository.TYPE,
(metadata) -> new AzureRepository(metadata, env, namedXContentRegistry, azureStoreService, threadPool));
(metadata) -> new AzureRepository(metadata, namedXContentRegistry, azureStoreService, threadPool));
}

@Override
Expand Down
3 changes: 1 addition & 2 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 @@ -26,7 +26,6 @@
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.threadpool.ThreadPool;

Expand All @@ -43,7 +42,7 @@ private AzureRepository azureRepository(Settings settings) {
.put(settings)
.build();
final AzureRepository azureRepository = new AzureRepository(new RepositoryMetaData("foo", "azure", internalSettings),
TestEnvironment.newEnvironment(internalSettings), NamedXContentRegistry.EMPTY, mock(AzureStorageService.class),
NamedXContentRegistry.EMPTY, mock(AzureStorageService.class),
mock(ThreadPool.class));
assertThat(azureRepository.getBlobStore(), is(nullValue()));
return azureRepository;
Expand Down
2 changes: 1 addition & 1 deletion ...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStoragePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ protected GoogleCloudStorageService createStorageService() {
public Map<String, Repository.Factory> getRepositories(Environment env, NamedXContentRegistry namedXContentRegistry,
ThreadPool threadPool) {
return Collections.singletonMap(GoogleCloudStorageRepository.TYPE,
metadata -> new GoogleCloudStorageRepository(metadata, env, namedXContentRegistry, this.storageService, threadPool));
metadata -> new GoogleCloudStorageRepository(metadata, namedXContentRegistry, this.storageService, threadPool));
}

@Override
Expand Down
11 changes: 6 additions & 5 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 @@ -28,7 +28,6 @@
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.repositories.RepositoryException;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
import org.elasticsearch.threadpool.ThreadPool;
Expand Down Expand Up @@ -61,10 +60,12 @@ class GoogleCloudStorageRepository extends BlobStoreRepository {
private final String bucket;
private final String clientName;

GoogleCloudStorageRepository(RepositoryMetaData metadata, Environment environment,
NamedXContentRegistry namedXContentRegistry,
GoogleCloudStorageService storageService, ThreadPool threadPool) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, buildBasePath(metadata));
GoogleCloudStorageRepository(
final RepositoryMetaData metadata,
final NamedXContentRegistry namedXContentRegistry,
final GoogleCloudStorageService storageService,
final ThreadPool threadPool) {
super(metadata, namedXContentRegistry, threadPool, buildBasePath(metadata));
this.storageService = storageService;

this.chunkSize = getSetting(CHUNK_SIZE, metadata);
Expand Down
2 changes: 1 addition & 1 deletion ...ins/repository-hdfs/src/main/java/org/elasticsearch/repositories/hdfs/HdfsRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public final class HdfsRepository extends BlobStoreRepository {

public HdfsRepository(RepositoryMetaData metadata, Environment environment,
NamedXContentRegistry namedXContentRegistry, ThreadPool threadPool) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, BlobPath.cleanPath());
super(metadata, namedXContentRegistry, threadPool, BlobPath.cleanPath());

this.environment = environment;
this.chunkSize = metadata.settings().getAsBytesSize("chunk_size", null);
Expand Down
12 changes: 6 additions & 6 deletions plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -159,11 +158,12 @@ class S3Repository extends BlobStoreRepository {
/**
* Constructs an s3 backed repository
*/
S3Repository(final RepositoryMetaData metadata,
final Settings settings,
final NamedXContentRegistry namedXContentRegistry,
final S3Service service, final ThreadPool threadPool) {
super(metadata, settings, namedXContentRegistry, threadPool, buildBasePath(metadata));
S3Repository(
final RepositoryMetaData metadata,
final NamedXContentRegistry namedXContentRegistry,
final S3Service service,
final ThreadPool threadPool) {
super(metadata, namedXContentRegistry, threadPool, buildBasePath(metadata));
this.service = service;

// Parse and validate the user's S3 Storage Class setting
Expand Down
12 changes: 6 additions & 6 deletions ...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,17 +76,17 @@ public S3RepositoryPlugin(final Settings settings) {
}

// proxy method for testing
protected S3Repository createRepository(final RepositoryMetaData metadata,
final Settings settings,
final NamedXContentRegistry registry, final ThreadPool threadPool) {
return new S3Repository(metadata, settings, registry, service, threadPool);
protected S3Repository createRepository(
final RepositoryMetaData metadata,
final NamedXContentRegistry registry,
final ThreadPool threadPool) {
return new S3Repository(metadata, registry, service, threadPool);
}

@Override
public Map<String, Repository.Factory> getRepositories(final Environment env, final NamedXContentRegistry registry,
final ThreadPool threadPool) {
return Collections.singletonMap(S3Repository.TYPE,
metadata -> createRepository(metadata, env.settings(), registry, threadPool));
return Collections.singletonMap(S3Repository.TYPE, metadata -> createRepository(metadata, registry, threadPool));
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...sitory-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,9 @@ public ProxyS3RepositoryPlugin(Settings settings) {
}

@Override
protected S3Repository createRepository(RepositoryMetaData metadata, Settings settings,
protected S3Repository createRepository(RepositoryMetaData metadata,
NamedXContentRegistry registry, ThreadPool threadPool) {
return new S3Repository(metadata, settings, registry, service, threadPool) {
return new S3Repository(metadata, registry, service, threadPool) {
@Override
protected void assertSnapshotOrGenericThread() {
// eliminate thread name check as we create repo manually on test/main threads
Expand Down
2 changes: 1 addition & 1 deletion ...sitory-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public TestS3RepositoryPlugin(final Settings settings) {
public Map<String, Repository.Factory> getRepositories(final Environment env, final NamedXContentRegistry registry,
final ThreadPool threadPool) {
return Collections.singletonMap(S3Repository.TYPE,
metadata -> new S3Repository(metadata, env.settings(), registry, new S3Service() {
metadata -> new S3Repository(metadata, registry, new S3Service() {
@Override
AmazonS3 buildClient(S3ClientSettings clientSettings) {
return new MockAmazonS3(blobs, bucket, serverSideEncryption, cannedACL, storageClass);
Expand Down
2 changes: 1 addition & 1 deletion plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public void testDefaultBufferSize() {
}

private S3Repository createS3Repo(RepositoryMetaData metadata) {
return new S3Repository(metadata, Settings.EMPTY, NamedXContentRegistry.EMPTY, new DummyS3Service(), mock(ThreadPool.class)) {
return new S3Repository(metadata, NamedXContentRegistry.EMPTY, new DummyS3Service(), mock(ThreadPool.class)) {
@Override
protected void assertSnapshotOrGenericThread() {
// eliminate thread name check as we create repo manually on test/main threads
Expand Down
14 changes: 6 additions & 8 deletions server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
*/
public static final Setting<Boolean> COMPRESS_SETTING = Setting.boolSetting("compress", true, Setting.Property.NodeScope);

private final Settings settings;

private final boolean compress;

private final RateLimiter snapshotRateLimiter;
Expand Down Expand Up @@ -201,12 +199,13 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
/**
* Constructs new BlobStoreRepository
* @param metadata The metadata for this repository including name and settings
* @param settings Settings for the node this repository object is created on
* @param threadPool Threadpool to run long running repository manipulations on asynchronously
*/
protected BlobStoreRepository(RepositoryMetaData metadata, Settings settings, NamedXContentRegistry namedXContentRegistry,
ThreadPool threadPool, BlobPath basePath) {
this.settings = settings;
protected BlobStoreRepository(
final RepositoryMetaData metadata,
final NamedXContentRegistry namedXContentRegistry,
final ThreadPool threadPool,
final BlobPath basePath) {
this.metadata = metadata;
this.threadPool = threadPool;
this.compress = COMPRESS_SETTING.get(metadata.settings());
Expand Down Expand Up @@ -678,8 +677,7 @@ private BlobContainer shardContainer(IndexId indexId, ShardId shardId) {
* @return rate limiter or null of no throttling is needed
*/
private RateLimiter getRateLimiter(Settings repositorySettings, String setting, ByteSizeValue defaultRate) {
ByteSizeValue maxSnapshotBytesPerSec = repositorySettings.getAsBytesSize(setting,
settings.getAsBytesSize(setting, defaultRate));
ByteSizeValue maxSnapshotBytesPerSec = repositorySettings.getAsBytesSize(setting, defaultRate);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the mistake that underpins this entire change. The use of settings.getAsByteSize(setting, defaultRate) is useless here. settings here is node-level settings, and these can never contain the throttle rates since they are not registered as node-level settings. So we always end up falling back to the default in the case the repository settings do not have a throttle defined. Once we remove this usage, we can remove settings from being a field in this class, and then the entire rest of the change set falls out.

if (maxSnapshotBytesPerSec.getBytes() <= 0) {
return null;
} else {
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public class FsRepository extends BlobStoreRepository {
*/
public FsRepository(RepositoryMetaData metadata, Environment environment, NamedXContentRegistry namedXContentRegistry,
ThreadPool threadPool) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, BlobPath.cleanPath());
super(metadata, namedXContentRegistry, threadPool, BlobPath.cleanPath());
this.environment = environment;
String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings());
if (location.isEmpty()) {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,7 @@ protected void assertSnapshotOrGenericThread() {
} else {
return metaData -> {
final Repository repository = new MockEventuallyConsistentRepository(
metaData, environment, xContentRegistry(), deterministicTaskQueue.getThreadPool(), blobStoreContext);
metaData, xContentRegistry(), deterministicTaskQueue.getThreadPool(), blobStoreContext);
repository.start();
return repository;
};
Expand Down
10 changes: 6 additions & 4 deletions ...c/test/java/org/elasticsearch/snapshots/mockstore/MockEventuallyConsistentRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.env.Environment;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
import org.elasticsearch.snapshots.SnapshotInfo;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -69,9 +68,12 @@ public class MockEventuallyConsistentRepository extends BlobStoreRepository {

private final NamedXContentRegistry namedXContentRegistry;

public MockEventuallyConsistentRepository(RepositoryMetaData metadata, Environment environment,
NamedXContentRegistry namedXContentRegistry, ThreadPool threadPool, Context context) {
super(metadata, environment.settings(), namedXContentRegistry, threadPool, BlobPath.cleanPath());
public MockEventuallyConsistentRepository(
final RepositoryMetaData metadata,
final NamedXContentRegistry namedXContentRegistry,
final ThreadPool threadPool,
final Context context) {
super(metadata, namedXContentRegistry, threadPool, BlobPath.cleanPath());
this.context = context;
this.namedXContentRegistry = namedXContentRegistry;
}
Expand Down
12 changes: 6 additions & 6 deletions ...t/java/org/elasticsearch/snapshots/mockstore/MockEventuallyConsistentRepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void setUp() throws Exception {
public void testReadAfterWriteConsistently() throws IOException {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();
final BlobContainer blobContainer = repository.blobStore().blobContainer(repository.basePath());
Expand All @@ -82,7 +82,7 @@ public void testReadAfterWriteConsistently() throws IOException {
public void testReadAfterWriteAfterReadThrows() throws IOException {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();
final BlobContainer blobContainer = repository.blobStore().blobContainer(repository.basePath());
Expand All @@ -98,7 +98,7 @@ public void testReadAfterWriteAfterReadThrows() throws IOException {
public void testReadAfterDeleteAfterWriteThrows() throws IOException {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();
final BlobContainer blobContainer = repository.blobStore().blobContainer(repository.basePath());
Expand All @@ -116,7 +116,7 @@ public void testReadAfterDeleteAfterWriteThrows() throws IOException {
public void testOverwriteRandomBlobFails() throws IOException {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();
final BlobContainer container = repository.blobStore().blobContainer(repository.basePath());
Expand All @@ -133,7 +133,7 @@ public void testOverwriteRandomBlobFails() throws IOException {
public void testOverwriteShardSnapBlobFails() throws IOException {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();
final BlobContainer container =
Expand All @@ -151,7 +151,7 @@ public void testOverwriteShardSnapBlobFails() throws IOException {
public void testOverwriteSnapshotInfoBlob() {
MockEventuallyConsistentRepository.Context blobStoreContext = new MockEventuallyConsistentRepository.Context();
try (BlobStoreRepository repository = new MockEventuallyConsistentRepository(
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY), environment,
new RepositoryMetaData("testRepo", "mockEventuallyConsistent", Settings.EMPTY),
xContentRegistry(), mock(ThreadPool.class), blobStoreContext)) {
repository.start();

Expand Down