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.

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
Closed

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2024

…m the settings builder

Description

Deletes code that removes number_of_routing_shards from settings builder during index creation. Alternatively to deleting this code, PR-14446 fixes the issue by inserting the field before returning the get settings response.

Related Issues

Resolves #14199

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

❌ Gradle check result for 88ea225: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 9433835: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@ghost ghost marked this pull request as ready for review June 18, 2024 23:50
@github-actions github-actions bot added bug Something isn't working good first issue Good for newcomers Indexing Indexing, Bulk Indexing and anything related to indexing labels Jun 18, 2024
Copy link
Contributor

❌ Gradle check result for 5db2c07: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Collaborator

@gaobinlong gaobinlong left a comment

Choose a reason for hiding this comment

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

Good work! But please amend your commits with -s for DCO, and make sure all gradle checks can pass.

CHANGELOG.md Outdated
@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Added
- Add fingerprint ingest processor ([#13724](https://github.com/opensearch-project/OpenSearch/pull/13724))
- [Remote Store] Rate limiter for remote store low priority uploads ([#14374](https://github.com/opensearch-project/OpenSearch/pull/14374/))
- Updated GET {index}/_settings to return `number_of_routing_shards` ([#14443](https://github.com/opensearch-project/OpenSearch/pull/14443))
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 this changelog should be in the Fixed section.

@@ -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!

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.

Copy link
Contributor

❌ Gradle check result for 5d066ba: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…m the settings builder

Signed-off-by: Sophia <tjdud6024@gmail.com>
Copy link
Contributor

❌ Gradle check result for 0a1a115: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c6b0ac8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@ghost
Copy link
Author

ghost commented Jun 19, 2024

Good work! But please amend your commits with -s for DCO, and make sure all gradle checks can pass.

Thank you! I've amended my commit for DCO but I'm not sure why the gradle check is failing. It looks like the tests are failing because of network connectivity?

java.net.ConnectException: Connect to http://[::1]:41687/ [/0:0:0:0:0:0:0:1] failed: Connection refused

@gaobinlong
Copy link
Collaborator

Good work! But please amend your commits with -s for DCO, and make sure all gradle checks can pass.

Thank you! I've amended my commit for DCO but I'm not sure why the gradle check is failing. It looks like the tests are failing because of network connectivity?

java.net.ConnectException: Connect to http://[::1]:41687/ [/0:0:0:0:0:0:0:1] failed: Connection refused

You can make some change for this PR(merge the main branch or add a new commit), then the gradle checks will run again.

@ghost ghost closed this Jun 21, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Indexing Indexing, Bulk Indexing and anything related to indexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] GET {index}/_settings does not list number_of_routing_shards as setting
1 participant