Skip to content
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

Use data stream for ILM history #64521

Merged
merged 21 commits into from
Nov 9, 2020

Conversation

probakowski
Copy link
Contributor

This change moves ILM history from using normal index and legacy template to data streams and composable templates.
It also unmutes several ITs to check if using data streams will resolve some race conditions there.

@probakowski
Copy link
Contributor Author

@elasticmachine update branch

@probakowski probakowski marked this pull request as ready for review November 5, 2020 16:08
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Nov 5, 2020
@probakowski
Copy link
Contributor Author

Note: this supersedes #60280 with basically cleaned up version of the same code

@@ -646,7 +646,8 @@ private void wipeCluster() throws Exception {
protected static void wipeAllIndices() throws IOException {
boolean includeHidden = minimumNodeVersion().onOrAfter(Version.V_7_7_0);
try {
final Request deleteRequest = new Request("DELETE", "*");
//remove all indices except ilm history which can pop up after deleting all data streams but shouldn't interfere
final Request deleteRequest = new Request("DELETE", "*,-.ds-ilm-history-*");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fishy but it's similar situation as we had before - ILM history index could pop up just after deleting all indices and it didn't do any harm

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for working on this Przemko. This is so important and impactful. Impressive how many tests are now fixed and how many flaky uses cases around aliases and bootstrappoing are not valid anymore 🚀

Comment on lines 68 to 70
if (ilmHistoryEnabled == false) {
throw new ElasticsearchException("can not index ILM history items when ILM history is disabled");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this check here as well.

Is this meant to catch the case when the ilm history was disabled between the time putAsync was called and the bulk was actually executed? If so, I think it'd be fine to allow it to go through as opposed to failing (and the next putAsync call will not record any history items anymore)

If we do decide to keep it I believe the exception should report the items it had not indexed.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the indices.lifecycle.history_index_enabled setting is not dynamic, I don't think we need this check (since putAsync will always punt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I've removed it

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left one comment, such a nice cleanup!

Comment on lines 68 to 70
if (ilmHistoryEnabled == false) {
throw new ElasticsearchException("can not index ILM history items when ILM history is disabled");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the indices.lifecycle.history_index_enabled setting is not dynamic, I don't think we need this check (since putAsync will always punt)

@probakowski
Copy link
Contributor Author

@elasticmachine update branch

@probakowski probakowski merged commit 9dedef0 into elastic:master Nov 9, 2020
@probakowski probakowski deleted the ilm-history-data-stream2 branch November 9, 2020 21:45
probakowski added a commit to probakowski/elasticsearch that referenced this pull request Nov 9, 2020
This change moves ILM history from using normal index and legacy template to data streams and composable templates.
It also unmutes several ITs to check if using data streams will resolve some race conditions there.
# Conflicts:
#	x-pack/plugin/ilm/src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ILMHistoryTests.java
#	x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryStore.java
probakowski added a commit that referenced this pull request Nov 9, 2020
* Use data stream for ILM history (#64521)

This change moves ILM history from using normal index and legacy template to data streams and composable templates.
It also unmutes several ITs to check if using data streams will resolve some race conditions there.
# Conflicts:
#	x-pack/plugin/ilm/src/internalClusterTest/java/org/elasticsearch/xpack/ilm/ILMHistoryTests.java
#	x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryStore.java

* fix unused imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management >enhancement Team:Data Management Meta label for data/management team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants