Skip to content

Commit

Permalink
Address PR Comments.
Browse files Browse the repository at this point in the history 
Signed-off-by: Harish Bhakuni <[email protected]>
  • Loading branch information
Harish Bhakuni committed Mar 15, 2024
1 parent 42da4aa commit f04da49
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 37 deletions.
20 changes: 17 additions & 3 deletions ...r/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,20 @@ public void testSnapshotRestoreOnRestrictReplicationSetting() throws Exception {

createSnapshot();

// Delete index
RestoreSnapshotResponse restoreSnapshotResponse = restoreSnapshotWithSettings(restoreIndexSegRepSettings(), RESTORED_INDEX_NAME);
assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED);
ensureGreen(RESTORED_INDEX_NAME);
GetSettingsResponse settingsResponse = client().admin()
.indices()
.getSettings(new GetSettingsRequest().indices(RESTORED_INDEX_NAME).includeDefaults(true))
.get();
assertEquals(settingsResponse.getSetting(RESTORED_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.SEGMENT.toString());

// Delete indices
assertAcked(client().admin().indices().delete(new DeleteIndexRequest(INDEX_NAME)).get());
assertFalse("index [" + INDEX_NAME + "] should have been deleted", indexExists(INDEX_NAME));
assertAcked(client().admin().indices().delete(new DeleteIndexRequest(RESTORED_INDEX_NAME)).get());
assertFalse("index [" + RESTORED_INDEX_NAME + "] should have been deleted", indexExists(RESTORED_INDEX_NAME));

// Start new set of nodes with cluster level replication type setting and restrict replication type setting.
Settings settings = Settings.builder()
Expand All @@ -366,17 +377,20 @@ public void testSnapshotRestoreOnRestrictReplicationSetting() throws Exception {
// Perform snapshot restore
logger.info("--> Performing snapshot restore to target index");

RestoreSnapshotResponse restoreSnapshotResponse = restoreSnapshotWithSettings(null);
restoreSnapshotResponse = restoreSnapshotWithSettings(null);

// Assertions
assertEquals(restoreSnapshotResponse.status(), RestStatus.ACCEPTED);
ensureGreen(RESTORED_INDEX_NAME);
GetSettingsResponse settingsResponse = client().admin()
settingsResponse = client().admin()
.indices()
.getSettings(new GetSettingsRequest().indices(RESTORED_INDEX_NAME).includeDefaults(true))
.get();
assertEquals(settingsResponse.getSetting(RESTORED_INDEX_NAME, SETTING_REPLICATION_TYPE), ReplicationType.SEGMENT.toString());

// restore index with cluster default setting
restoreSnapshotWithSettings(restoreIndexSegRepSettings(), RESTORED_INDEX_NAME + "1");

// Perform Snapshot Restore with different index name
IllegalArgumentException exception = expectThrows(
IllegalArgumentException.class,
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@ public static void updateReplicationStrategy(
final ReplicationType indexReplicationType;
if (INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings)) {
indexReplicationType = INDEX_REPLICATION_TYPE_SETTING.get(requestSettings);
} else if (INDEX_REPLICATION_TYPE_SETTING.exists(combinedTemplateSettings)) {
} else if (combinedTemplateSettings != null && INDEX_REPLICATION_TYPE_SETTING.exists(combinedTemplateSettings)) {
indexReplicationType = INDEX_REPLICATION_TYPE_SETTING.get(combinedTemplateSettings);
} else if (CLUSTER_REPLICATION_TYPE_SETTING.exists(clusterSettings)) {
indexReplicationType = CLUSTER_REPLICATION_TYPE_SETTING.get(clusterSettings);
Expand Down
47 changes: 14 additions & 33 deletions server/src/main/java/org/opensearch/snapshots/RestoreService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
import org.opensearch.index.store.remote.filecache.FileCacheStats;
import org.opensearch.indices.IndicesService;
import org.opensearch.indices.ShardLimitValidator;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.node.remotestore.RemoteStoreNodeAttribute;
import org.opensearch.repositories.IndexId;
import org.opensearch.repositories.RepositoriesService;
Expand All @@ -113,7 +112,6 @@
import java.util.stream.Collectors;

import static java.util.Collections.unmodifiableSet;
import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REPLICATION_TYPE_SETTING;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_CREATION_DATE;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_HISTORY_UUID;
Expand All @@ -123,17 +121,14 @@
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_UPGRADED;
import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY;
import static org.opensearch.common.util.set.Sets.newHashSet;
import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING;
import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION;
import static org.opensearch.index.store.remote.filecache.FileCache.DATA_TO_FILE_CACHE_SIZE_RATIO_SETTING;
import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING;
import static org.opensearch.node.Node.NODE_SEARCH_CACHE_SIZE_SETTING;
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.isRemoteStoreAttributePresent;
import static org.opensearch.snapshots.SnapshotUtils.filterIndices;

/**
Expand Down Expand Up @@ -177,6 +172,7 @@ public class RestoreService implements ClusterStateApplier {

// It's OK to change some settings, but we shouldn't allow simply removing them
private static final Set<String> USER_UNREMOVABLE_SETTINGS;
private static final String REMOTE_STORE_INDEX_SETTINGS_REGEX = "index.remote_store.*";

static {
Set<String> unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 4);
Expand Down Expand Up @@ -669,43 +665,28 @@ private String[] getIgnoreSettingsInternal() {
// related index settings present in the snapshot.
String[] indexSettingsToBeIgnored = new String[] {};
if (false == RemoteStoreNodeAttribute.isRemoteStoreAttributePresent(clusterService.getSettings())) {
indexSettingsToBeIgnored = ArrayUtils.concat(indexSettingsToBeIgnored, new String[] { "index.remote_store.*" });
indexSettingsToBeIgnored = ArrayUtils.concat(
indexSettingsToBeIgnored,
new String[] { REMOTE_STORE_INDEX_SETTINGS_REGEX }
);
}
return indexSettingsToBeIgnored;
}

private Settings getOverrideSettingsInternal() {
final Settings.Builder settingsBuilder = Settings.builder();
updateReplicationStrategy(
MetadataCreateIndexService.updateReplicationStrategy(
settingsBuilder,
request.indexSettings(),
clusterService.getSettings(),
clusterService.getClusterSettings()
null
);
// remote store settings needs to be overridden if the remote store feature is enabled in the
// cluster where snapshot is being restored.
MetadataCreateIndexService.updateRemoteStoreSettings(settingsBuilder, clusterService.getSettings());
return settingsBuilder.build();
}

private void updateReplicationStrategy(
Settings.Builder settingsBuilder,
Settings requestSettings,
Settings nodeSettings,
ClusterSettings clusterSettings
) {
// if remote store feature is enabled, override replication strategy to segment.
// if `cluster.index.restrict.replication.type` is enabled and user have not provided
// replication strategy setting with restore request, override replication strategy
// setting to cluster default.
if (isRemoteStoreAttributePresent(nodeSettings)) {
settingsBuilder.put(SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT);
} else if (!INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings)
&& clusterSettings.get(IndicesService.CLUSTER_INDEX_RESTRICT_REPLICATION_TYPE_SETTING)) {
settingsBuilder.put(SETTING_REPLICATION_TYPE, clusterSettings.get(CLUSTER_REPLICATION_TYPE_SETTING).name());
}
}

private void populateIgnoredShards(String index, final Set<Integer> ignoreShards) {
for (SnapshotShardFailure failure : snapshotInfo.shardFailures()) {
if (index.equals(failure.index())) {
Expand Down Expand Up @@ -778,18 +759,18 @@ private void validateExistingIndex(
*/
private IndexMetadata updateIndexSettings(
IndexMetadata indexMetadata,
Settings changeSettings,
Settings overrideSettings,
String[] ignoreSettings,
Settings changeSettingsInternal,
Settings overrideSettingsInternal,
String[] ignoreSettingsInternal
) {
Settings normalizedChangeSettings = Settings.builder()
.put(changeSettings)
.put(overrideSettings)
.normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX)
.build();
if (IndexSettings.INDEX_SOFT_DELETES_SETTING.get(indexMetadata.getSettings())
&& IndexSettings.INDEX_SOFT_DELETES_SETTING.exists(changeSettings)
&& IndexSettings.INDEX_SOFT_DELETES_SETTING.get(changeSettings) == false) {
&& IndexSettings.INDEX_SOFT_DELETES_SETTING.exists(overrideSettings)
&& IndexSettings.INDEX_SOFT_DELETES_SETTING.get(overrideSettings) == false) {
throw new SnapshotRestoreException(
snapshot,
"cannot disable setting [" + IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey() + "] on restore"
Expand Down Expand Up @@ -849,8 +830,8 @@ private IndexMetadata updateIndexSettings(
}));

// override internal settings
if (changeSettingsInternal != null) {
settingsBuilder.put(changeSettingsInternal).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX);
if (overrideSettingsInternal != null) {
settingsBuilder.put(overrideSettingsInternal).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX);
}
settingsBuilder.remove(MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING.getKey());
return builder.settings(settingsBuilder).build();
Expand Down

0 comments on commit f04da49

Please sign in to comment.