Skip to content

Commit

Permalink
Address PR Comments.
Browse files Browse the repository at this point in the history 
Signed-off-by: Harish Bhakuni <hbhakuni@amazon.com>
  • Loading branch information
Harish Bhakuni committed Mar 15, 2024
1 parent 42da4aa commit 215f51c
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 32 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
39 changes: 11 additions & 28 deletions server/src/main/java/org/opensearch/snapshots/RestoreService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,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 +670,25 @@ 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 +761,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 +832,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 215f51c

Please sign in to comment.