-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Remote Store] Add support to provide separate segment metadata repository #12993
base: main
Are you sure you want to change the base?
Conversation
eb0918f
to
0dea8c1
Compare
Compatibility status:Checks if related components are compatible with change 95d18c7 Incompatible componentsIncompatible components: [https://github.com/opensearch-project/cross-cluster-replication.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/performance-analyzer.git] |
❌ Gradle check result for 0dea8c1: 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? |
0dea8c1
to
940ce2e
Compare
❌ Gradle check result for 940ce2e: 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 eb0918f: ABORTED 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? |
940ce2e
to
87ce493
Compare
❌ Gradle check result for 87ce493: 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 a844c8a: 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? |
a844c8a
to
506d6b4
Compare
❕ Gradle check result for d260b1c: 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12993 +/- ##
============================================
+ Coverage 71.42% 71.44% +0.02%
- Complexity 59978 60419 +441
============================================
Files 4985 5026 +41
Lines 282275 284475 +2200
Branches 40946 41202 +256
============================================
+ Hits 201603 203237 +1634
- Misses 63999 64389 +390
- Partials 16673 16849 +176 ☔ View full report in Codecov by Sentry. |
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.
Overall LGTM, with increased repositories whats the impact to node attributes and join validations?
server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>
2852ca4
to
1391737
Compare
❌ Gradle check result for 1391737: 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? |
Signed-off-by: Sachin Kale <[email protected]>
❌ Gradle check result for 95d18c7: 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? |
SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, | ||
SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY | ||
SETTING_REMOTE_SEGMENT_STORE_DATA_REPOSITORY, | ||
SETTING_REMOTE_TRANSLOG_STORE_DATA_REPOSITORY |
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.
do we need to add the metadata repo as well to unmodified settings during snapshot restore?
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.
+1. We will need corresponding ITs as well to restore .
@@ -2702,7 +2702,7 @@ public void snapshotRemoteStoreIndexShard( | |||
indexTotalNumberOfFiles, | |||
indexTotalFileSize, | |||
store.indexSettings().getUUID(), | |||
store.indexSettings().getRemoteStoreRepository(), | |||
store.indexSettings().getRemoteSegmentStoreDataRepository(), |
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.
i think we need to add another parameter for metadata repository as well since lock files are in metadata repo. currently the remoteStoreRepository
is used for both source data and lock files.
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.
Right! Thanks for catching this, let me make the required change.
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.
Lock files are stored separately stored and not in metadata repo .
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.
Lock file path is generated with segment repository when data and metadata is the same repo.
Once we separate data and metadata, we need to associate lock files with one of these repos. As per the comment: #12993 (comment), it makes more sense to have lock files stored in metadata repo.
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.
We will need to add sourceRemoteStoreMetadataRepository
as well to RestoreSnapshotRequest
.
@@ -407,6 +407,7 @@ void recoverFromSnapshotAndRemoteStore( | |||
threadPool | |||
); | |||
RemoteSegmentStoreDirectory sourceRemoteDirectory = (RemoteSegmentStoreDirectory) directoryFactory.newDirectory( | |||
remoteStoreRepository, |
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.
so, right now we pull this remote store repository value from shard level snapshot metadata, we need to make related changes there to update that with metadata directory now.
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.
Also, i don't like the idea of using same repository name here for metadata and data, we should maybe create a remote directory instance from directoryFactory which have access to metadata repository only.
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.
checked more on this, actually we will need segments_N file for restoring snapshot, which is in data directory, so we will need to add both segment data and metadata repository info in snapshot metadata. unless we keep segments_N file in metadata directory as well.
@@ -903,6 +903,7 @@ public static void remoteDirectoryCleanup( | |||
) { | |||
try { | |||
RemoteSegmentStoreDirectory remoteSegmentStoreDirectory = (RemoteSegmentStoreDirectory) remoteDirectoryFactory.newDirectory( | |||
remoteStoreRepoForIndex, |
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.
here as well, as mentioned in above comment, we can create the same remoteDirectory instance which have access only to metadata repository.
@@ -407,6 +407,7 @@ void recoverFromSnapshotAndRemoteStore( | |||
threadPool | |||
); |
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.
Along with the doc changes of this feature, we need to update doc for source_remote_store_repository
which will be now used to override remote segment metadata directory: https://opensearch.org/docs/latest/api-reference/snapshots/restore-snapshot/
This PR is stalled because it has been open for 30 days with no activity. |
Description
Related Issues
Check List
New functionality has been documented.New functionality has javadoc addedFailing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)Public documentation issue/PR createdBy 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.