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

removed logic that deleted INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING fro… #14443

Closed
wants to merge 1 commit into from
+39 −1
Closed

removed logic that deleted INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING fro… #14443

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
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 @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Fixed
- Fix handling of Short and Byte data types in ScriptProcessor ingest pipeline ([#14379](https://github.com/opensearch-project/OpenSearch/issues/14379))
- Switch to iterative version of WKT format parser ([#14086](https://github.com/opensearch-project/OpenSearch/pull/14086))
- Updated GET {index}/_settings to return `number_of_routing_shards` ([#14443](https://github.com/opensearch-project/OpenSearch/pull/14443))

### Security

Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ IndexMetadata buildAndValidateTemporaryIndexMetadata(

// remove the setting it's temporary and is only relevant once we create the index
final Settings.Builder settingsBuilder = Settings.builder().put(aggregatedIndexSettings);
settingsBuilder.remove(IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.getKey());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess this is intended, index.number_of_routing_shards couldn't exist in the settings of IndexMetadata because it's a static setting, can only be set when creating index, so we added a new field routing_num_shards outside the index settings to the IndexMetadata, change this may cause unexpected behavior.

Copy link
Author

Choose a reason for hiding this comment

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

If removing this seems risky, I have another draft solution that doesn't require removing this line. Would it be better to go forward with this approach instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the solution in the draft PR is better.

Copy link
Author

Choose a reason for hiding this comment

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

Oh okay sounds good! I will get that PR ready and close this one out!


final Settings indexSettings = settingsBuilder.build();

final IndexMetadata.Builder tmpImdBuilder = IndexMetadata.builder(request.index());
Expand Down
37 changes: 37 additions & 0 deletions server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,43 @@ public void testRemoteCustomData() {
);
}

public void testBuildAndValidateTemporaryIndexMetadata() {
int routingShards = 3;
withTemporaryClusterService(((clusterService, threadPool) -> {
MetadataCreateIndexService checkerService = new MetadataCreateIndexService(
Settings.EMPTY,
clusterService,
indicesServices,
null,
null,
createTestShardLimitService(randomIntBetween(1, 1000), false, clusterService),
null,
null,
threadPool,
null,
new SystemIndices(Collections.emptyMap()),
false,
new AwarenessReplicaBalance(Settings.EMPTY, clusterService.getClusterSettings()),
DefaultRemoteStoreSettings.INSTANCE,
repositoriesServiceSupplier
);

Settings indexSettings = Settings.builder()
.put("index.version.created", Version.CURRENT)
.put(INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 3)
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)
.put(IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.getKey(), routingShards)
.build();

CreateIndexClusterStateUpdateRequest request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");

IndexMetadata indexMetadata = checkerService.buildAndValidateTemporaryIndexMetadata(indexSettings, request, routingShards);

assertEquals(indexMetadata.getSettings(), indexSettings);
assertEquals(indexMetadata.getRoutingNumShards(), routingShards);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add a yaml test case to verify the Get index settings API can return index.number_of_routing_shards.

}));
}

private IndexMetadata testRemoteCustomData(boolean remoteStoreEnabled, PathType pathType) {
Settings.Builder settingsBuilder = Settings.builder();
if (remoteStoreEnabled) {
Expand Down
Loading