Skip to content

Commit

Permalink
Introduce string constant for readonly setting (#68291)
Browse files Browse the repository at this point in the history
A blob store repository can be put in readonly mode by setting
`readonly: true` in its settings. In the codebase the setting key is
just the literal string `"readonly"` wherever it's used and it takes
some effort to determine what the right setting name is, in particular
to check each time that it's not spelled `"read_only"`.

This commit replaces those literal `"readonly"` strings with the
`BlobStoreRepository#READONLY_SETTING_KEY` constant to reduce this
trappiness.
  • Loading branch information
DaveCTurner committed Feb 1, 2021
1 parent b765e70 commit 0dba407
Show file tree
Hide file tree
Showing 13 changed files with 37 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public static final class Repository {
public static final Setting<ByteSizeValue> CHUNK_SIZE_SETTING =
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);
public static final Setting<Boolean> READONLY_SETTING = Setting.boolSetting("readonly", false, Property.NodeScope);
public static final Setting<Boolean> READONLY_SETTING = Setting.boolSetting(READONLY_SETTING_KEY, false, Property.NodeScope);
}

private final BlobPath basePath;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
import org.elasticsearch.test.ESTestCase;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.mock;
Expand All @@ -57,7 +58,7 @@ public void testReadonlyDefault() {

public void testReadonlyDefaultAndReadonlyOn() {
assertThat(azureRepository(Settings.builder()
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.build()).isReadOnly(), is(true));
}

Expand All @@ -70,35 +71,35 @@ public void testReadonlyWithPrimaryOnly() {
public void testReadonlyWithPrimaryOnlyAndReadonlyOn() {
assertThat(azureRepository(Settings.builder()
.put(AzureRepository.Repository.LOCATION_MODE_SETTING.getKey(), LocationMode.PRIMARY_ONLY.name())
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.build()).isReadOnly(), is(true));
}

public void testReadonlyWithSecondaryOnlyAndReadonlyOn() {
assertThat(azureRepository(Settings.builder()
.put(AzureRepository.Repository.LOCATION_MODE_SETTING.getKey(), LocationMode.SECONDARY_ONLY.name())
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.build()).isReadOnly(), is(true));
}

public void testReadonlyWithSecondaryOnlyAndReadonlyOff() {
assertThat(azureRepository(Settings.builder()
.put(AzureRepository.Repository.LOCATION_MODE_SETTING.getKey(), LocationMode.SECONDARY_ONLY.name())
.put("readonly", false)
.put(READONLY_SETTING_KEY, false)
.build()).isReadOnly(), is(false));
}

public void testReadonlyWithPrimaryAndSecondaryOnlyAndReadonlyOn() {
assertThat(azureRepository(Settings.builder()
.put(AzureRepository.Repository.LOCATION_MODE_SETTING.getKey(), LocationMode.PRIMARY_THEN_SECONDARY.name())
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.build()).isReadOnly(), is(true));
}

public void testReadonlyWithPrimaryAndSecondaryOnlyAndReadonlyOff() {
assertThat(azureRepository(Settings.builder()
.put(AzureRepository.Repository.LOCATION_MODE_SETTING.getKey(), LocationMode.PRIMARY_THEN_SECONDARY.name())
.put("readonly", false)
.put(READONLY_SETTING_KEY, false)
.build()).isReadOnly(), is(false));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.util.Map;
import java.util.stream.Collectors;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -285,8 +286,8 @@ private void ensureSnapshotRestoreWorks(String repoName, String name, int shards

private static void createRepository(RestHighLevelClient client, String repoName, boolean readOnly) throws IOException {
assertThat(client.snapshot().createRepository(new PutRepositoryRequest(repoName).type("fs").settings(
Settings.builder().put("location", "./" + repoName).put("readonly", readOnly)), RequestOptions.DEFAULT).isAcknowledged(),
is(true));
Settings.builder().put("location", "./" + repoName).put(READONLY_SETTING_KEY, readOnly)), RequestOptions.DEFAULT)
.isAcknowledged(), is(true));
}

private static void createSnapshot(RestHighLevelClient client, String repoName, String name, String index) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.Arrays;
import java.util.function.Function;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -126,7 +127,7 @@ public void testConcurrentWipeAndRecreateFromOtherCluster() throws InterruptedEx
secondCluster.startDataOnlyNode();
assertAcked(secondCluster.client().admin().cluster().preparePutRepository(repoName)
.setType("fs")
.setSettings(Settings.builder().put("location", repoPath).put("readonly", true)));
.setSettings(Settings.builder().put("location", repoPath).put(READONLY_SETTING_KEY, true)));
assertThat(secondCluster.client().admin().cluster().prepareGetRepositories(repoName).get().repositories()
.stream().filter(r -> r.name().equals(repoName)).findFirst().get().uuid(), equalTo(repoUuid));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.nio.file.Path;
import java.util.List;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertRequestBuilderThrows;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -183,7 +184,7 @@ public void testRepositoryVerification() throws Exception {
.put("location", randomRepoPath())
.put("random_control_io_exception_rate", 1.0).build();
Settings readonlySettings = Settings.builder().put(settings)
.put("readonly", true).build();
.put(READONLY_SETTING_KEY, true).build();
logger.info("--> creating repository that cannot write any files - should fail");
assertRequestBuilderThrows(client.admin().cluster().preparePutRepository("test-repo-1")
.setType("mock").setSettings(settings),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import static org.elasticsearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING;
import static org.elasticsearch.index.shard.IndexShardTests.getEngineFromShard;
import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAllSuccessful;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
Expand Down Expand Up @@ -912,7 +913,7 @@ public void testReadonlyRepository() throws Exception {
createRepository("readonly-repo", "fs", Settings.builder()
.put("location", repositoryLocation)
.put("compress", randomBoolean())
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES));
logger.info("--> restore index after deletion");
RestoreSnapshotResponse restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot("readonly-repo", "test-snap")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp

private static final String UPLOADED_DATA_BLOB_PREFIX = "__";

/**
* All {@link BlobStoreRepository} implementations can be made read-only by setting this key to {@code true} in their settings.
*/
public static final String READONLY_SETTING_KEY = "readonly";

/**
* Prefix used for the identifiers of data blobs that were not actually written to the repository physically because their contents are
* already stored in the metadata referencing them, i.e. in {@link BlobStoreIndexShardSnapshot} and
Expand Down Expand Up @@ -337,7 +342,7 @@ protected BlobStoreRepository(
this.supportURLRepo = SUPPORT_URL_REPO.get(metadata.settings());
snapshotRateLimiter = getRateLimiter(metadata.settings(), "max_snapshot_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB));
restoreRateLimiter = getRateLimiter(metadata.settings(), "max_restore_bytes_per_sec", ByteSizeValue.ZERO);
readOnly = metadata.settings().getAsBoolean("readonly", false);
readOnly = metadata.settings().getAsBoolean(READONLY_SETTING_KEY, false);
cacheRepositoryData = CACHE_REPOSITORY_DATA.get(metadata.settings());
bufferSize = Math.toIntExact(BUFFER_SIZE_SETTING.get(metadata.settings()).getBytes());
this.maxSnapshotCount = MAX_SNAPSHOTS_SETTING.get(metadata.settings());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import java.util.Map;
import java.util.Set;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -101,7 +102,7 @@ protected final String createRepository(final String name, final Settings settin
internalCluster().getDataOrMasterNodeInstances(RepositoriesService.class).forEach(repositories -> {
assertThat(repositories.repository(name), notNullValue());
assertThat(repositories.repository(name), instanceOf(BlobStoreRepository.class));
assertThat(repositories.repository(name).isReadOnly(), is(settings.getAsBoolean("readonly", false)));
assertThat(repositories.repository(name).isReadOnly(), is(settings.getAsBoolean(READONLY_SETTING_KEY, false)));
BlobStore blobStore = ((BlobStoreRepository) repositories.repository(name)).getBlobStore();
assertThat("blob store has to be lazy initialized", blobStore, verify ? is(notNullValue()) : is(nullValue()));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.nio.file.Path;
import java.util.stream.Stream;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -76,7 +77,7 @@ public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOExce
}
assertFalse(Files.exists(deletedPath));

createRepository(repoName, Settings.builder().put(repoSettings).put("readonly", true).build(), randomBoolean());
createRepository(repoName, Settings.builder().put(repoSettings).put(READONLY_SETTING_KEY, true).build(), randomBoolean());

final ElasticsearchException exception = expectThrows(ElasticsearchException.class, () ->
client().admin().cluster().prepareRestoreSnapshot(repoName, snapshotName).setWaitForCompletion(randomBoolean()).get());
Expand All @@ -90,7 +91,7 @@ public void testReadOnly() throws Exception {
final Path repoPath = randomRepoPath();
final Settings repoSettings = Settings.builder()
.put(repositorySettings(repoName))
.put("readonly", true)
.put(READONLY_SETTING_KEY, true)
.put(FsRepository.LOCATION_SETTING.getKey(), repoPath)
.put(BlobStoreRepository.BUFFER_SIZE_SETTING.getKey(), String.valueOf(randomIntBetween(1, 8) * 1024) + "kb")
.build();
Expand All @@ -107,7 +108,7 @@ public void testReadOnly() throws Exception {
assertFalse(Files.exists(storePath));
}

createRepository(repoName, Settings.builder().put(repoSettings).put("readonly", false).build(), false);
createRepository(repoName, Settings.builder().put(repoSettings).put(READONLY_SETTING_KEY, false).build(), false);

try (BlobStore store = newBlobStore(repoName)) {
assertTrue(Files.exists(repoPath));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
import java.util.function.Predicate;
import java.util.stream.StreamSupport;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -139,7 +140,7 @@ public void assertRepoConsistency() {
if (skipRepoConsistencyCheckReason == null) {
clusterAdmin().prepareGetRepositories().get().repositories().forEach(repositoryMetadata -> {
final String name = repositoryMetadata.name();
if (repositoryMetadata.settings().getAsBoolean("readonly", false) == false) {
if (repositoryMetadata.settings().getAsBoolean(READONLY_SETTING_KEY, false) == false) {
clusterAdmin().prepareDeleteSnapshot(name, OLD_VERSION_SNAPSHOT_PREFIX + "*").get();
clusterAdmin().prepareCleanupRepository(name).get();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Locale;
import java.util.stream.Stream;

import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.repositories.encrypted.EncryptedRepository.getEncryptedBlobByteLength;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -137,7 +138,7 @@ public void testTamperedEncryptionMetadata() throws Exception {
});
}

final Settings settings = Settings.builder().put(repoSettings).put("readonly", randomBoolean()).build();
final Settings settings = Settings.builder().put(repoSettings).put(READONLY_SETTING_KEY, randomBoolean()).build();
final boolean verifyOnCreate = randomBoolean();

if (verifyOnCreate) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import java.util.stream.Collectors;

import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.contains;
Expand Down Expand Up @@ -740,7 +741,7 @@ protected String createRepository(final String name, final Settings settings, fi
internalCluster().getDataOrMasterNodeInstances(RepositoriesService.class).forEach(repositories -> {
assertThat(repositories.repository(name), notNullValue());
assertThat(repositories.repository(name), instanceOf(BlobStoreRepository.class));
assertThat(repositories.repository(name).isReadOnly(), is(settings.getAsBoolean("readonly", false)));
assertThat(repositories.repository(name).isReadOnly(), is(settings.getAsBoolean(READONLY_SETTING_KEY, false)));
});

return name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING;
import static org.elasticsearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING;
import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
import static org.elasticsearch.repositories.blobstore.BlobStoreRepository.READONLY_SETTING_KEY;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.DATA_TIERS_PREFERENCE;
Expand Down Expand Up @@ -942,7 +943,7 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr
assertAcked(client().admin().cluster().prepareDeleteRepository(repositoryName));
final Settings.Builder settings = Settings.builder().put(repositoryMetadata.settings());
if (randomBoolean()) {
settings.put("readonly", "true");
settings.put(READONLY_SETTING_KEY, "true");
}
assertAcked(
clusterAdmin().preparePutRepository(newRepositoryName)
Expand Down

0 comments on commit 0dba407

Please sign in to comment.