-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Move force merge from the downsmapling request to the ILM action #135834
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
Move force merge from the downsmapling request to the ILM action #135834
Conversation
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
This idea won't work, because we cannot influence the segments of the downsampled index. |
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
Pinging @elastic/es-data-management (Team:Data Management) |
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, just left a few small comments :)
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
private boolean isForceMergeEnabled(Map<String, Object> sourceIndexMappings) { | ||
if (sourceIndexMappings.containsKey("_meta")) { | ||
if (sourceIndexMappings.get("_meta") instanceof Map<?, ?> metadataMap) { | ||
var enabledForceMergeValue = metadataMap.get("downsample.forcemerge.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.
Just double-checking: do we have any documentation on this "setting"? It's in a weird place so I doubt we care much about it, but just wanted to double-check that we're ok with not supporting this configuration any more.
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.
This setting is not documented on purpose. We added it to allow users that cannot upgrade yet from 8.19
, to test the impact of skipping the force-merge request. It's just a workaround since the proper way, which we implement with this PR, cannot be backported.
...multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/DownsampleActionIT.java
Show resolved
Hide resolved
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 also in general. I have the same question as Niels about not looking at the metadata any more, is that going to be considered a breaking change, since a user that has that configured as false
would start having their downsampled indices force merged after this change?
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
This is an undocumented workaround to allow users in |
Hi @gmarouli, I've created a changelog YAML for you. |
@dakrone and/or @nielsbauman can you take another look? |
Hi @gmarouli, I've updated the changelog YAML for you. |
In this PR, we propose to add practical tips for downsampling. For now this includes, a guideline on how to choose the downsampling interval. And then specifically for ILM, an explanation on how downsampling relates with tiers. After elastic/elasticsearch#135834, we should also add here the option to disable force merge. --------- Co-authored-by: Kostas Krikellas <[email protected]> Co-authored-by: Marci W <[email protected]>
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. The only recommendation I have left is to run the test classes you changed a few thousand iterations, to ensure there's no combination of random values that results in a test failure.
Apologies for the late review, and thanks a lot for working and iterating on this!
...n/core/src/main/java/org/elasticsearch/xpack/core/action/TimeSeriesUsageTransportAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DownsampleAction.java
Outdated
Show resolved
Hide resolved
...n/core/src/main/java/org/elasticsearch/xpack/core/action/TimeSeriesUsageTransportAction.java
Outdated
Show resolved
Hide resolved
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 left one small typo suggestion, other than that LGTM!
...n/core/src/main/java/org/elasticsearch/xpack/core/action/TimeSeriesUsageTransportAction.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Niels Bauman <[email protected]>
Thank you for the review @nielsbauman , no worries about the delay, there were many things in progress so I wasn't blocked! |
In this PR we move the force-merge operation from the downsampling request to the ILM action.
Our goal is to decouple the downsampling operation from the force-merge operation. With this change the downsampling request is responsible to ensure that the downsampled index is refreshed and flushed but not to force merge it.
We believe that most of the time this is not necessary, and executing the force-merge operation unnecessarily can increase the load on the cluster.
To preserve backwards compatibility we move the responsibility to execute the existing force merge to the downsample ILM action and we make it configurable. By default, it will run but a user can disable it just as they can with a searchable snapshot.
Update
As a follow up of this PR, we pose the question is the force merge in the downsample action intentional and useful?
To answer this question, we extend time series telemetry. We define that the force merge step in the downsample ILM action is useful, if this is the only force merge step operation before a searchable snapshot.
Effectively, by this definition, we argue that the force merge in downsampling is not an intentional operation the user has requested but only the result of the implementation. We identify the biggest impact of removing it to be a searchable snapshot, but if the searchable snapshot performs its own force merge (and more performant force merge #133954) then we could skip this operation in the downsample action altogether.
Fixes: #135618