-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support for custom remote store segment path prefix to support clusterless configurations #18857
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18857 +/- ##
============================================
- Coverage 72.88% 72.79% -0.10%
+ Complexity 69160 69110 -50
============================================
Files 5633 5633
Lines 317978 318008 +30
Branches 45988 45993 +5
============================================
- Hits 231752 231487 -265
- Misses 67517 67744 +227
- Partials 18709 18777 +68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@msfroh - Could you please review? |
|
❌ Gradle check result for 706e564: 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? |
|
❌ Gradle check result for 08d63f2: 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? |
|
❌ Gradle check result for 9102895: 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? |
|
❌ Gradle check result for 6aaa438: 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? |
|
❕ Gradle check result for 3095f53: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
msfroh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rgsriram! This looks really good. I just had two easy-to-fix comments:
- Use
PrivateIndexinstead ofInternalIndexfor the new setting, for consistency with other remote store settings. - Rename the parameter in
ShardDataPathInputto more generically represent the fact that it's an index-level fixed prefix, without necessarily assuming that it comes from the writer node ID.
Thanks!
server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Fixed the breaking change Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Added all the tests Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Removed unwanted comments Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Removed unwanted comments Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Added necessary comments for the code made for this change in RemoteStorePathStrategyTests Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Refactored the code Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Refactored the code Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Refactored the code Signed-off-by: Sriram Ganesh <srignsh22@gmail.com>
Signed-off-by: Sriram Ganesh <srignsh22@gmail.com>
Signed-off-by: Michael Froh <msfroh@apache.org>
…usterless configurations (opensearch-project#18857) --------- Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Signed-off-by: Michael Froh <msfroh@apache.org> Co-authored-by: Michael Froh <msfroh@apache.org>
…usterless configurations (opensearch-project#18857) --------- Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Signed-off-by: Michael Froh <msfroh@apache.org> Co-authored-by: Michael Froh <msfroh@apache.org>
…usterless configurations (opensearch-project#18857) --------- Signed-off-by: Sriram Ganesh <srignsh22@gmail.com> Signed-off-by: Michael Froh <msfroh@apache.org> Co-authored-by: Michael Froh <msfroh@apache.org>
Description
This commit adds support for injecting a unique writer node identifier into remote store paths to support clusterless configurations where multiple writers may write to the same shard.
Problem:
In clusterless configurations, multiple writers can write to the same shard simultaneously, which would cause conflicts with the current remote store path structure that doesn't include writer identification.
Solution:
index.remote_store.segment.path_prefixthat allows injecting a writer node ID into the remote store pathImplementation Details:
INDEX_REMOTE_STORE_SEGMENT_PATH_PREFIXsetting in IndexMetadata with proper validation that ensures:RemoteStorePathStrategyto include writer node ID in path generation between shard ID and data categoryIndexSettingsto handle the new setting with proper null/empty checksRemoteSegmentStoreDirectoryFactoryandRemoteStoreLockManagerFactoryto pass writer node ID through the path generation chainPath Structure:
Before:
remote_repository/hash/index-uuid/shard-id/segments/data/fileAfter:
remote_repository/hash/index-uuid/shard-id/writer-node-id/segments/data/fileUsage:
{ "index.remote_store.segment.path_prefix": "writer-node-1" }Validation:
index.remote_store.enabledis trueRelated Issues
Resolves #18750
Check List
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.