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

[Remote Store] Add index specific setting for remote repository #4253

Merged
merged 3 commits into from
Sep 6, 2022
Merged

[Remote Store] Add index specific setting for remote repository #4253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19885b7
Add index specific setting for remote repository
Aug 18, 2022
6b1f527
Add changelog entry
Sep 1, 2022
f6be982
Fix for failover incremental uploads
Sep 5, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use RemoteSegmentStoreDirectory instead of RemoteDirectory ([#4240](https://github.com/opensearch-project/OpenSearch/pull/4240))
- Plugin ZIP publication groupId value is configurable ([#4156](https://github.com/opensearch-project/OpenSearch/pull/4156))
- Update to Netty 4.1.80.Final ([#4359](https://github.com/opensearch-project/OpenSearch/pull/4359))
- Add index specific setting for remote repository ([#4253](https://github.com/opensearch-project/OpenSearch/pull/4253))

### Deprecated

Expand Down
58 changes: 48 additions & 10 deletions server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,8 @@ public Iterator<Setting<?>> settings() {

public static final String SETTING_REMOTE_STORE_ENABLED = "index.remote_store.enabled";

public static final String SETTING_REMOTE_STORE_REPOSITORY = "index.remote_store.repository";

public static final String SETTING_REMOTE_TRANSLOG_STORE_ENABLED = "index.remote_store.translog.enabled";
/**
* Used to specify if the index data should be persisted in the remote store.
Expand Down Expand Up @@ -322,6 +324,50 @@ public Iterator<Setting<?>> settings() {
Property.Final
);

/**
* Used to specify remote store repository to use for this index.
*/
public static final Setting<String> INDEX_REMOTE_STORE_REPOSITORY_SETTING = Setting.simpleString(
SETTING_REMOTE_STORE_REPOSITORY,
new Setting.Validator<>() {

@Override
public void validate(final String value) {}

@Override
public void validate(final String value, final Map<Setting<?>, Object> settings) {
if (value == null || value.isEmpty()) {
throw new IllegalArgumentException(
"Setting " + INDEX_REMOTE_STORE_REPOSITORY_SETTING.getKey() + " should be provided with non-empty repository ID"
);
} else {
validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_STORE_REPOSITORY_SETTING);
}
}

Comment on lines +338 to +347
Copy link
Collaborator

@Bukhtawar Bukhtawar Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to validate if the repository is valid and exists essentially checking against all the list of registered repositories

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can add that check. Index creation will anyway fail if repository does not exist but it will take time to fail and will not provide helpful error message.

Let me add a check for repository exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bukhtawar To check against actual repository, we need to inject RepositoriesService into IndexMetadata whose builder/constructor mostly takes index settings related parameters (like numberOfShards, requireFilters, indexCreatedVersion). IMO, RepositoriesService injection would be an outlier. Need to think more on that. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might have to support overloaded constructor or builders since I believe now we are associating a new attribute to IndexMetadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have created a tracking issue here to handle this: #4403

Copy link
Collaborator

@Bukhtawar Bukhtawar Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Mandatorily passing a remote store repo per index seems to be adding a lot of friction.
Thinking out loud, how does this durability semantics work if one index has HDFS other has S3. I am not quite sure if this would complicate things in future.

Copy link
Member Author

@sachinpkale sachinpkale Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Got it. Created tracking issue here: #4409

@Override
public Iterator<Setting<?>> settings() {
final List<Setting<?>> settings = Collections.singletonList(INDEX_REMOTE_STORE_ENABLED_SETTING);
return settings.iterator();
}
},
Property.IndexScope,
Property.Final
);

private static void validateRemoteStoreSettingEnabled(final Map<Setting<?>, Object> settings, Setting<?> setting) {
final Boolean isRemoteSegmentStoreEnabled = (Boolean) settings.get(INDEX_REMOTE_STORE_ENABLED_SETTING);
if (isRemoteSegmentStoreEnabled == false) {
throw new IllegalArgumentException(
"Settings "
+ setting.getKey()
+ " can ont be set/enabled when "
+ INDEX_REMOTE_STORE_ENABLED_SETTING.getKey()
+ " is set to true"
);
}
}

/**
* Used to specify if the index translog operations should be persisted in the remote store.
*/
Expand All @@ -335,16 +381,8 @@ public void validate(final Boolean value) {}

@Override
public void validate(final Boolean value, final Map<Setting<?>, Object> settings) {
final Boolean isRemoteSegmentStoreEnabled = (Boolean) settings.get(INDEX_REMOTE_STORE_ENABLED_SETTING);
if (isRemoteSegmentStoreEnabled == false && value == true) {
throw new IllegalArgumentException(
"Settings "
+ INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING.getKey()
+ " cannot be enabled when "
+ INDEX_REMOTE_STORE_ENABLED_SETTING.getKey()
+ " is set to "
+ settings.get(INDEX_REMOTE_STORE_ENABLED_SETTING)
);
if (value == true) {
validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING);
}
}

Expand Down
6 changes: 5 additions & 1 deletion server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,11 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
FeatureFlags.REPLICATION_TYPE,
Collections.singletonList(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING),
FeatureFlags.REMOTE_STORE,
Arrays.asList(IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING, IndexMetadata.INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING)
Arrays.asList(
IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING,
IndexMetadata.INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING,
IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING
)
);

public static final IndexScopedSettings DEFAULT_SCOPED_SETTINGS = new IndexScopedSettings(Settings.EMPTY, BUILT_IN_INDEX_SETTINGS);
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/index/IndexService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ public synchronized IndexShard createShard(
Store remoteStore = null;
if (this.indexSettings.isRemoteStoreEnabled()) {
Directory remoteDirectory = remoteDirectoryFactory.newDirectory(
clusterService.state().metadata().clusterUUID(),
this.indexSettings.getRemoteStoreRepository(),
this.indexSettings,
path
);
Expand Down
9 changes: 9 additions & 0 deletions server/src/main/java/org/opensearch/index/IndexSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ public final class IndexSettings {
private final ReplicationType replicationType;
private final boolean isRemoteStoreEnabled;
private final boolean isRemoteTranslogStoreEnabled;
private final String remoteStoreRepository;
// volatile fields are updated via #updateIndexMetadata(IndexMetadata) under lock
private volatile Settings settings;
private volatile IndexMetadata indexMetadata;
Expand Down Expand Up @@ -721,6 +722,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
replicationType = ReplicationType.parseString(settings.get(IndexMetadata.SETTING_REPLICATION_TYPE));
isRemoteStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false);
isRemoteTranslogStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_ENABLED, false);
remoteStoreRepository = settings.get(IndexMetadata.SETTING_REMOTE_STORE_REPOSITORY);
this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings);
this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings);
this.queryStringAnalyzeWildcard = QUERY_STRING_ANALYZE_WILDCARD.get(nodeSettings);
Expand Down Expand Up @@ -979,6 +981,13 @@ public boolean isRemoteTranslogStoreEnabled() {
return isRemoteTranslogStoreEnabled;
}

/**
* Returns if remote store is enabled for this index.
*/
public String getRemoteStoreRepository() {
return remoteStoreRepository;
}

Comment on lines +986 to +990
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it come with a default?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it would be null.

/**
* Returns the node settings. The settings returned from {@link #getSettings()} are a merged version of the
* index settings and the node settings where node settings are overwritten by index settings.
Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ public RemoteStoreRefreshListener(IndexShard indexShard) {
.getDelegate()).getDelegate();
this.primaryTerm = indexShard.getOperationPrimaryTerm();
localSegmentChecksumMap = new HashMap<>();
if (indexShard.shardRouting.primary()) {
try {
this.remoteDirectory.init();
} catch (IOException e) {
logger.error("Exception while initialising RemoteSegmentStoreDirectory", e);
}
}
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 code block is added to address bug mentioned here: #4398

Comment on lines +62 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this logic be sitting inside indexShard.remoteStore().directory() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is present in initialization of RemoteSegmentStoreDirectory. But when IndexShard is created with remote directory, during failover, only engine of the shard changes. For new primary, this engine would be InternalEngine and new instance of RemoteStoreRefreshListener is created but the remote directory instance is still old (at the time of IndexShard creation). So the remote directory instance is not aware of segments that were uploaded by old primary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me think more on abstracting out init() to prevent misuse. Created issue to track this: #4410

}

@Override
Expand Down
69 changes: 68 additions & 1 deletion server/src/test/java/org/opensearch/index/IndexSettingsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ public void testEnablingRemoteTranslogStoreFailsWhenRemoteSegmentDisabled() {
() -> IndexMetadata.INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING.get(indexSettings)
);
assertEquals(
"Settings index.remote_store.translog.enabled cannot be enabled when index.remote_store.enabled is set to false",
"Settings index.remote_store.translog.enabled can ont be set/enabled when index.remote_store.enabled is set to true",
iae.getMessage()
);
}
Expand All @@ -876,4 +876,71 @@ public void testEnablingRemoteStoreFailsWhenReplicationTypeIsDefault() {
);
assertEquals("To enable index.remote_store.enabled, index.replication.type should be set to SEGMENT", iae.getMessage());
}

public void testRemoteRepositoryDefaultSetting() {
IndexMetadata metadata = newIndexMeta(
"index",
Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT).build()
);
IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY);
assertNull(settings.getRemoteStoreRepository());
}

public void testRemoteRepositoryExplicitSetting() {
IndexMetadata metadata = newIndexMeta(
"index",
Settings.builder()
.put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)
.put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, true)
.put(IndexMetadata.SETTING_REMOTE_STORE_REPOSITORY, "repo1")
.build()
);
IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY);
assertEquals("repo1", settings.getRemoteStoreRepository());
}

public void testUpdateRemoteRepositoryFails() {
Set<Setting<?>> remoteStoreSettingSet = new HashSet<>();
remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING);
IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet);
IllegalArgumentException error = expectThrows(
IllegalArgumentException.class,
() -> settings.updateSettings(
Settings.builder().put("index.remote_store.repository", randomUnicodeOfLength(10)).build(),
Settings.builder(),
Settings.builder(),
"index"
)
);
assertEquals(error.getMessage(), "final index setting [index.remote_store.repository], not updateable");
}

public void testSetRemoteRepositoryFailsWhenRemoteStoreIsNotEnabled() {
Settings indexSettings = Settings.builder()
.put("index.replication.type", ReplicationType.SEGMENT)
.put("index.remote_store.enabled", false)
.put("index.remote_store.repository", "repo1")
.build();
IllegalArgumentException iae = expectThrows(
IllegalArgumentException.class,
() -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings)
);
assertEquals(
"Settings index.remote_store.repository can ont be set/enabled when index.remote_store.enabled is set to true",
iae.getMessage()
);
}

public void testSetRemoteRepositoryFailsWhenEmptyString() {
Settings indexSettings = Settings.builder()
.put("index.replication.type", ReplicationType.SEGMENT)
.put("index.remote_store.enabled", false)
.put("index.remote_store.repository", "")
.build();
IllegalArgumentException iae = expectThrows(
IllegalArgumentException.class,
() -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings)
);
assertEquals("Setting index.remote_store.repository should be provided with non-empty repository ID", iae.getMessage());
}
}