Add fast copy-constructor for adding index to Metadata#87863
Merged
original-brownbear merged 9 commits intoelastic:mainfrom Sep 20, 2022
original-brownbear:faster-add-index
Merged
Add fast copy-constructor for adding index to Metadata#87863original-brownbear merged 9 commits intoelastic:mainfrom original-brownbear:faster-add-index
original-brownbear merged 9 commits intoelastic:mainfrom
original-brownbear:faster-add-index
Conversation
Member
|
Is this still something we're pursuing @original-brownbear? |
Contributor
Author
|
Yea let me clean this up real quick, with all the recent fixes this one is a 10%+ speedup in benchmark bootstrap time actually ... would be nice to have this |
Collaborator
|
Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner
approved these changes
Sep 20, 2022
Member
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, a couple of nits
It'd be nice to have a way to assert that a Metadata satisfies all the invariants we expect of it just to make sure we don't mess anything up when adding copy-constructors like this (now or in future)
server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/ArrayUtils.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Thanks David, I applied your NITs and I agree on the assertions. I'll follow up for this, I'd like to do a bigger PR here where we just move all the asserts from the current builder to the constructor, that should give us a neat safety net. |
original-brownbear
added a commit
that referenced
this pull request
Nov 9, 2022
) This fixes a bug introduced in #87863 which added a Metadata copy constructor with separate name collision checks that assumed index name and alias names were already validated in `IndexMetada`. => fixed corrupted states by actually adding the validation to `IndexMetadata` to make it impossible to instantiate a broken `IndexMetadata` in the first place.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Nov 9, 2022
) (#91469) This fixes a bug introduced in #87863 which added a Metadata copy constructor with separate name collision checks that assumed index name and alias names were already validated in `IndexMetada`. => fixed corrupted states by actually adding the validation to `IndexMetadata` to make it impossible to instantiate a broken `IndexMetadata` in the first place.
original-brownbear
added a commit
that referenced
this pull request
Nov 29, 2022
…es bug (#91993) * Fix corrupted Metadata from index and alias having the same name (#91456) This fixes a bug introduced in #87863 which added a Metadata copy constructor with separate name collision checks that assumed index name and alias names were already validated in `IndexMetada`. => fixed corrupted states by actually adding the validation to `IndexMetadata` to make it impossible to instantiate a broken `IndexMetadata` in the first place. * Implement repair functionality for aliases colliding with indices bug (#91887) Follow up to #91456 implementing an automated fix for indices corrupted in 8.5.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a fast copy constructor for creating an index. The building of new metadata is taking more than 15% of CPU time on the master node during many shards benchmark bootstrapping and completely disappears from profiling with this change.
It's unfortunate that we don't really have a nice way of testing this logic directly since we don't have a clean equality check on metadata but it has massive coverage through its use for almost all index creates (except for those using a builder filter function) in all ITs so this seems safe enough to me.