[ML] Refactor ML mappings and templates into JSON resources by dimitris-athanasiou · Pull Request #51765 · elastic/elasticsearch

dimitris-athanasiou · 2020-01-31T17:30:17Z

ML mappings and index templates have so far been created
programmatically. While this had its merits due to static typing,
there is consensus it would be clear to maintain those in json files.
In addition, we are going to adding ILM policies to these indices
and the component for a plugin to register ILM policies is
IndexTemplateRegistry. It expects the templates to be in resource
json files.

For the above reasons this commit refactors ML mappings and index
templates into json resource files that are registered via
MlIndexTemplateRegistry.

elasticmachine · 2020-01-31T17:30:19Z

Pinging @elastic/ml-core (:ml)

dimitris-athanasiou · 2020-01-31T17:31:32Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java

@gwbrown Here are the changes I made to allow variable replacement on template loading. Could you take a look please when you have some time?

dimitris-athanasiou · 2020-01-31T17:32:42Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java

I have not replaced this with returning an operator that does nothing yet because it is unclear whether IndexTemplateRegistry fully addresses upgrading issues. I'm waiting to hear on this, until then the PR is WIP.

dimitris-athanasiou · 2020-01-31T17:34:11Z

...gin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProviderTests.java

These tests are unfortunately the result of extreme mocking, something we tried in the past but doesn't seem to bring benefits. I have removed them and replaced them with integration tests in JobResultsProviderIT.

dimitris-athanasiou · 2020-01-31T17:34:40Z

...gin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProviderTests.java

I just noticed this one, I forgot to address it. I'll take care of it before removing the WIP label.

dimitris-athanasiou · 2020-01-31T17:35:38Z

...ck/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/persistence/MockClientBuilder.java

In hindsight I'm not sure this mock client class was useful. I remove all unused methods.

dimitris-athanasiou · 2020-01-31T17:37:03Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlIndexTemplateRegistry.java

This template's name is formed by the inference prefix plus the latest version. This would mean that once we change version we'll create a new template. I think that's probably not a good idea. I left it as is for now but we can address in this PR. In fact, our template naming is a bit inconsistent but I'm not sure we can iron it easily. I'll make sure we discuss this.

AthenaEryma

Left a couple comments about escaping in the changes to TemplateUtils/IndexTemplateConfig, but I think once those are addressed the additional variable support will be valuable.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/IndexTemplateConfig.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java

droberts195

I saw a few nits plus there's a question of what we should do when the plugin is disabled.

Let me know if the test failures are showing up any unforeseen edge cases that need talking through.

droberts195 · 2020-02-03T15:20:18Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java

For the templates we decided to leave this out in the future. Can it be omitted here too now we're explicitly setting it in the affected tests? (Plus the code above that looks for whether we're using the black hole autodetect process.)

droberts195 · 2020-02-03T15:30:12Z

x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/meta_index_template.json

Should this be a variable?

droberts195 · 2020-02-03T17:05:47Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlIndexTemplateRegistry.java

We're using the same state index for data frame analytics state too, so maybe drop the ANOMALY_DETECTION_ bit?

I'd like to move all state-related logic in AnomalyDetectorsIndex into a MlStateIndex class. But I'd rather do that in a follow-up PR.

droberts195 · 2020-02-03T17:17:35Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlIndexTemplateRegistry.java

This is a difference compared to what we did with the old getIndexTemplateMetaDataUpgrader() functionality. In that we used to return our templates even when ML was disabled on the node. Returning nothing when ML is disabled isn't necessarily wrong, but it's a difference that needs thinking about.

There may be some subtle effect in Cloud for example. I think at one time there was a proposal that when a Cloud cluster gets switched to different hardware and a full cluster snapshot from the old hardware is restored into brand new empty cluster running on different hardware that all the X-Pack plugins are disabled during that restore. I am not sure if this is still being proposed or not. But it's a scenario where the ML plugin would be disabled in a cluster that contained ML indices.

Let's consult more widely on this offline.

It was wrong to return an empty list. A number of tests was failing because of that. As long as the plugin is loaded, the templates should be there as some nodes may become ML nodes.

hendrikmuhs

Looks good, added 2 comments.

hendrikmuhs · 2020-02-04T12:31:20Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java

there is a lot of conversions and deep copies: utf8->utf16->utf8->utf16. It does not look to hard to implement replaceVariables and validate with strings instead of utf8 byte buffers.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java

AthenaEryma · 2020-02-04T23:13:30Z

The changes in b765370 LGTM. Thank you!

dimitris-athanasiou · 2020-02-05T08:35:39Z

I believe the failing tests in MlConfigIndexMappingsFullClusterRestartIT are now because of the lack of template upgrading. This is worked on in #51456. Once that is merged I should rebase this PR and then I think it'll be ready.

ML mappings and index templates have so far been created programmatically. While this had its merits due to static typing, there is consensus it would be clear to maintain those in json files. In addition, we are going to adding ILM policies to these indices and the component for a plugin to register ILM policies is `IndexTemplateRegistry`. It expects the templates to be in resource json files. For the above reasons this commit refactors ML mappings and index templates into json resource files that are registered via `MlIndexTemplateRegistry`.

dimitris-athanasiou · 2020-02-12T10:50:41Z

This has now been rebased to pick #51456. Let's see if it passes all the tests.

AthenaEryma · 2020-02-12T21:43:14Z

Changes in 0aba76e also LGTM. Thanks again!

hendrikmuhs

LGTM

hendrikmuhs · 2020-02-13T09:22:54Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java

            throw new ElasticsearchParseException("Template must not be null");
        }
+        if (Strings.isEmpty(source)) {
+            throw new ElasticsearchParseException("Template must not be empty");


nit: I wonder about that: this exception type is used if a REST request is malformed. In this context this makes no sense and we consider the input immutable, right? IMHO this is more of a RTE kind of thing.

Anyway, this is out of scope for this PR, you just add another exception and follow the existing style.

hendrikmuhs · 2020-02-13T09:27:35Z

...ugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json

@@ -0,0 +1,36 @@
+{


can we put comments in such files? I think it would be good to leave some explanation to the reader of this. I would also add a placeholder for a changelog, in case we change the schema.

^ applies to all json files introduced

There is no support for comments in JSON files as it stands. We could in theory support them by removing them comment lines when such files are loaded. As for a changelog, I think one of the benefits for those being files, is that we treat them similarly to code files. Thus, git serves as a change log. We can git-blame them and see the commit where each line was introduced/modified. Not sure we need more. But let's see how it goes and we add them if it appears to be needed?

droberts195

LGTM

Since the CI is green and this change is blocking two other pieces of work from starting I would be happy to merge this as-is and do any additional minor tweaks in followup PRs.

…stic#51765) ML mappings and index templates have so far been created programmatically. While this had its merits due to static typing, there is consensus it would be clear to maintain those in json files. In addition, we are going to adding ILM policies to these indices and the component for a plugin to register ILM policies is `IndexTemplateRegistry`. It expects the templates to be in resource json files. For the above reasons this commit refactors ML mappings and index templates into json resource files that are registered via `MlIndexTemplateRegistry`. Backport of elastic#51765

#52353) ML mappings and index templates have so far been created programmatically. While this had its merits due to static typing, there is consensus it would be clear to maintain those in json files. In addition, we are going to adding ILM policies to these indices and the component for a plugin to register ILM policies is `IndexTemplateRegistry`. It expects the templates to be in resource json files. For the above reasons this commit refactors ML mappings and index templates into json resource files that are registered via `MlIndexTemplateRegistry`. Backport of #51765

dimitris-athanasiou added WIP >refactoring :ml Machine learning v8.0.0 v7.7.0 labels Jan 31, 2020

dimitris-athanasiou commented Jan 31, 2020

View reviewed changes

AthenaEryma reviewed Jan 31, 2020

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/IndexTemplateConfig.java Outdated Show resolved Hide resolved

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/TemplateUtils.java Outdated Show resolved Hide resolved

droberts195 reviewed Feb 3, 2020

View reviewed changes

hendrikmuhs reviewed Feb 4, 2020

View reviewed changes

dimitris-athanasiou mentioned this pull request Feb 5, 2020

Convert ILM and SLM histories into hidden indices #51456

Merged

dimitris-athanasiou force-pushed the refactor-ml-mappings-into-json-files branch from 3b553d8 to 7ace93c Compare February 12, 2020 10:49

dimitris-athanasiou removed the WIP label Feb 12, 2020

Force upgrades to happen on master node

0aba76e

hendrikmuhs approved these changes Feb 13, 2020

View reviewed changes

droberts195 approved these changes Feb 13, 2020

View reviewed changes

dimitris-athanasiou merged commit 8fcbd1d into elastic:master Feb 13, 2020

dimitris-athanasiou deleted the refactor-ml-mappings-into-json-files branch February 13, 2020 09:51

dimitris-athanasiou mentioned this pull request Feb 14, 2020

[7.x][ML] Refactor ML mappings and templates into JSON resources (#51… #52353

Merged

tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 23, 2020

Fix template due to elastic#51765

45df122

droberts195 mentioned this pull request Mar 25, 2020

[CI] TooManyJobsIT.testCloseFailedJob fails on 7.x #54162

Closed

davidkyle mentioned this pull request Apr 6, 2020

Wait for ML templates after creating a new cluster in TooManyJobsIT #54793

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

droberts195 mentioned this pull request Nov 23, 2021

Remove obsolete typed legacy index templates #80937

Merged

Conversation

dimitris-athanasiou commented Jan 31, 2020

Uh oh!

elasticmachine commented Jan 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitris-athanasiou Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AthenaEryma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AthenaEryma commented Feb 4, 2020

Uh oh!

dimitris-athanasiou commented Feb 5, 2020

Uh oh!

dimitris-athanasiou commented Feb 12, 2020

Uh oh!

AthenaEryma commented Feb 12, 2020

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dimitris-athanasiou Jan 31, 2020 •

edited

Loading

hendrikmuhs Feb 13, 2020 •

edited

Loading