Downsample ILM action should skip non-time-series indices#94835
Downsample ILM action should skip non-time-series indices#94835csoulios merged 8 commits intoelastic:mainfrom
Conversation
|
Hi @csoulios, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Pinging @elastic/es-data-management (Team:Data Management) |
| /** | ||
| * Updates the lifecycle policy for the rollup index for the original/currently managed index | ||
| */ | ||
| public class UpdateRollupIndexPolicyStep extends AsyncActionStep { |
There was a problem hiding this comment.
This step is a left over from a previous implementation of rollup v2. No longer used.
There was a problem hiding this comment.
Ah nice, thanks for deleting this
andreidan
left a comment
There was a problem hiding this comment.
Thanks for working on this Christos
Left a few minor comments
| /** | ||
| * Updates the lifecycle policy for the rollup index for the original/currently managed index | ||
| */ | ||
| public class UpdateRollupIndexPolicyStep extends AsyncActionStep { |
There was a problem hiding this comment.
Ah nice, thanks for deleting this
| private void createIndex(String index, String alias) throws IOException { | ||
| @Before | ||
| public void updateClusterSettings() throws IOException { | ||
| updateClusterSettings(client(), Settings.builder().put("indices.lifecycle.poll_interval", "5s").build()); |
There was a problem hiding this comment.
Is this something we want? What's the purpose of delaying the poll interval to later than the 1 second we configure it in tests?
There was a problem hiding this comment.
For some reason the @After cleanUpCluster() method sets the following:
[2023-03-29T13:19:01,928][INFO ][o.e.c.s.ClusterSettings ] [runTask-0] updating [indices.lifecycle.poll_interval] from [5s] to [10m]
So, I need to reset it at my @Before test.
So my logs look like this:
2023-03-29T13:19:01,649][INFO ][o.e.c.m.MetadataDeleteIndexService] [runTask-0] [index-xpirenuavc/DJ7v787aSwyGeKUjIhkzHg] deleting index
[2023-03-29T13:19:01,928][INFO ][o.e.c.s.ClusterSettings ] [runTask-0] updating [indices.lifecycle.poll_interval] from [5s] to [10m]
[2023-03-29T13:19:17,100][INFO ][o.e.c.s.ClusterSettings ] [runTask-0] updating [indices.lifecycle.poll_interval] from [10m] to [5s]
There was a problem hiding this comment.
I just ran the test on your branch (after having removed this @Before method) and I don't see the setting being changed to 10m.
Gradle configures the poll interval to 1000ms for these test clusters https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/ilm/qa/multi-node/build.gradle#L29 (elasticsearch.yml for all nodes in the cluster will contain this setting)
This is my diff for your branch
--- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/DownsampleActionIT.java
+++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/DownsampleActionIT.java
@@ -109,11 +109,6 @@ public class DownsampleActionIT extends ESRestTestCase {
);
}
- @Before
- public void updateClusterSettings() throws IOException {
- updateClusterSettings(client(), Settings.builder().put("indices.lifecycle.poll_interval", "5s").build());
- }
-
private void createIndex(String index, String alias, boolean isTimeSeries) throws IOException {
Settings.Builder settings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
Could you remove this and try again please? I'm fairly convinced we don't need it
| String rollupIndex = waitAndGetRollupIndexName(client(), index); | ||
| assertNull("Rollup index should not have been created", rollupIndex); | ||
| assertBusy(() -> assertTrue("Source index should not have been deleted", indexExists(index))); | ||
| assertBusy(() -> assertThat(getStepKeyForIndex(client(), index), equalTo(PhaseCompleteStep.finalStep(phaseName).getKey()))); |
There was a problem hiding this comment.
These assertions make sense, but they are written in a way that would make the test idle for 60 seconds until waitAndGetRollupIndexName timeouts.
Would it make sense to have the final step assertion be the first one ?
i.e.
assertBusy(() -> assertThat(getStepKeyForIndex(client(), index), equalTo(PhaseCompleteStep.finalStep(phaseName).getKey())));
Followed by regular assertions (they don't need to be assertBusy anymore as ILM is in the final step already)
e.g.
assertNull("Rollup index should not have been created", rollupIndex);
assertTrue("Source index should not have been deleted", indexExists(index));
💚 Backport successful
|
) Make the downsample action skip the non-time-series indices As discussed in elastic#93123 (comment): ... we think the downsample action should skip backing indices that are not part of a time series data stream. We do a similar approach with the unfollow action where non follower indices are skipped. And this allows to have the same policy for both regular data streams and tsdb data streams. Closes elastic#93123
…94902) Make the downsample action skip the non-time-series indices As discussed in #93123 (comment): ... we think the downsample action should skip backing indices that are not part of a time series data stream. We do a similar approach with the unfollow action where non follower indices are skipped. And this allows to have the same policy for both regular data streams and tsdb data streams. Closes #93123
Test DownsampleActionIT.testRollupIndex() has been failing after PR #94835 got merged. Most probably, because The PR removed the following line from the testRollupIndex() method ``` updateClusterSettings(client(), Settings.builder().put("indices.lifecycle.poll_interval", "5s").build()); ``` To fix the test, we set the indices.lifecycle.poll_interval setting before all tests run. Closes #95156
Make the
downsampleaction skip the non-time-series indicesAs discussed in #93123 (comment):
Closes #93123