-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Only store single reference to mapping for indices that share the same mappings in the cluster state #19929
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
base: main
Are you sure you want to change the base?
Conversation
…e mappings in the cluster state Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
|
This seems like low hanging fruit to optimize cluster state heap utilization esp in clusters with many rolled over indices. Is there anything I'm missing with this? |
|
❌ Gradle check result for 66fe01d: 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: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
|
MappingMetadata can only have a single Single mapping type is declared here: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/mapper/MapperService.java#L126 |
|
❌ Gradle check result for 4937d4a: 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 4937d4a: 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? |
| return this; | ||
| } | ||
|
|
||
| private void canonicalizeMappingsInPlace() { |
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 don't see a case where we are deleting a index, because it can happen that the mappings object we are holding in memory is deleted as part of the delete index call, then all other indices with same mapping pointing to null effectively.
Is that not the case ?
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.
The logic added here is within the metadata builder so the indices object in this builder would be only the present indices. I need to check, but on index deletion it would create a new version of the metadata using this builder with all of the indices except the one that has been deleted. This function is then called in build() to de-dup the mappings so that we don't hold multiple versions of the same mappings in-memory for the new in-memory Metadata.
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.
Description
This PR contains an optimization for the cluster state that would particularly be beneficial in cases of index rollover where many indices in the cluster share the same mappings.
This is another (and probably the largest) optimization along with 2 other related PRs:
For this optimization, it works at the time that the new Metadata instance is built to try de-duplicating the mappings if multiple indices share the same mappings.
Related Issues
Related to:
<index>/_mappingsrather than_mappingsAPI OpenSearch-Dashboards#10550Check 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.