-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Improving performance of get data streams API by avoiding getting effective mappings #138948
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
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @masseyke, I've created a changelog YAML for you. |
dakrone
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.
LGTM, I left one comment, thanks for fixing this.
| template, | ||
| state.metadata().componentTemplates() | ||
| ); | ||
| final Settings settings = templateSettings.merge(dataStream.getSettings()); |
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 this makes sense. Can we also add something like:
assert settings.equals(metadataDataStreamsSettingsService.getEffectiveSettings(state.metadata(), dataStream)) : "these should always match etc etc";after this line, so that if we ever encountered a situation where a provider changed the settings, then we would fail tests? That way we only pay the cost when asserts are enabled.
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 think that that assertion would work, b/c the providers do change some of the settings.
| * settings so that we can get some ilm information and the index mode, neither of which come from additional settings | ||
| * providers. |
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 index mode, neither of which come from additional settings
Comment providers.
I don't think that is true; LogsdbIndexModeSettingsProvider seems to supply an index mode:
Line 133 in 4c33d20
| additionalSettings.put(IndexSettings.MODE.getKey(), IndexMode.LOGSDB.getName()); |
Or am I misreading something?
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.
Oh good catch. That complicates things.
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'm thinking then that I'll still call getEffectiveSettings, but a new variant that lets you skip merging all the mappings.
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.
Oh, actually that next line that calls resolveMode actually applies all of the additional settings providers and we get the correct mode anyway. I'll add a test that shows that.
💚 Backport successful
|
The get data streams API needs the data stream's settings in order to get information about ILM and the index mode. It calls
MetadataDataStreamsService::getEffectiveSettingsto do this, which is correct. However, that method is fairly expensive because it callsgetEffectiveMappingsso that it can calladdSettingsFromIndexSettingProviderswith the correct mappings. We don't need any of the information from these index settings providers for ILM or the index mode. Since the get data streams API can be called for all data streams, we call this slow method hundreds or thousands of times, and the API can take so long to return that kibana gives up on it.This PR removes the call to getEffectiveSettings, and instead just gathers the settings from the template, along with data stream settings overrides.
This was caused by #137407