Use data stream for ILM history #60280
Conversation
probakowski
added
>enhancement
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-features (:Core/Features/ILM+SLM) |
elasticmachine
added
the
Team:Data Management
There was a problem hiding this comment.
Thanks for working on this @probakowski
Left a few comments
| @@ -59,7 +53,6 @@ | |||
| public class ILMHistoryStore implements Closeable { | |||
| private static final Logger logger = LogManager.getLogger(ILMHistoryStore.class); | |||
|
|
|||
| public static final String ILM_HISTORY_INDEX_PREFIX = "ilm-history-" + INDEX_TEMPLATE_VERSION + "-"; | |||
| public static final String ILM_HISTORY_ALIAS = "ilm-history-" + INDEX_TEMPLATE_VERSION; | |||
There was a problem hiding this comment.
should we rename this to _DATA_STREAM ?
| .filter(BulkItemResponse::isFailed) | ||
| .collect(Collectors.toMap(BulkItemResponse::getId, BulkItemResponse::getFailureMessage)); | ||
| logger.error("failures: [{}]", failures); | ||
| .map(r->r.getFailure().getCause()) |
There was a problem hiding this comment.
I'm not sure we should change the way we report errors. eg. could the failure cause be null ever? if so addSuppressed would fail. I think we should keep the BulkItemResponse#getFailureMessage as the reported error.
What do you think?
| // The index didn't exist before we made the call, there was probably a race - just ignore this | ||
| logger.debug("index [{}] was created after checking for its existence, likely due to a concurrent call", | ||
| initialHistoryIndexName); | ||
| if(e instanceof IllegalArgumentException && e.getMessage().contains("exists")){ |
There was a problem hiding this comment.
should we maybe probe the error message to be an exact match? ie. data_stream [ilm-history-3] already exists (with corresponding parameterized data stream name)
I wonder why we're not throwing ResourceAlreadyExists in the data stream case too.
There was a problem hiding this comment.
Changed this to throw ResourceAlreadyExistsException in #60518
| @@ -229,85 +218,21 @@ public void testHistoryIndexProperlyExistsAlready() throws InterruptedException | |||
| ElasticsearchAssertions.awaitLatch(latch, 10, TimeUnit.SECONDS); | |||
| } | |||
|
|
|||
| public void testHistoryIndexHasNoWriteIndex() throws InterruptedException { | |||
There was a problem hiding this comment.
pretty neat we can get rid of these edge cases 🎉
|
So I have a thought about this. We recently were discussing adding an API to migrate a set on indices (with an alias) into a data stream. Do we know if Kibana can correctly handle the scenario where |
| }, | ||
| "priority": 2147483647, | ||
| "version": ${xpack.ilm_history.template.version} |
There was a problem hiding this comment.
Since we support _meta here now, can we add:
"_meta": {
"managed": true,
"description": "index template for ILM indices indices"
}So that the UI can mark this accordingly?
| .setWaitForActiveShards(1) | ||
| .addAlias(new Alias(ILM_HISTORY_ALIAS).writeIndex(true).isHidden(true)) | ||
| .execute(new ActionListener<>() { | ||
| static void ensureHistoryDataStream(Client client, ClusterState state, ActionListener<Boolean> listener) { |
There was a problem hiding this comment.
So I think we should remove this entirely, part of the reason we have data streams now is to not have to pre set up anything before indexing. Now that we wouldn't use aliases we don't have the complicated alias setup that requires us checking this every time.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
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.