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

[ML] Make ml internal indices hidden #52423

Merged
merged 10 commits into from
Feb 19, 2020

Conversation

przemekwitek
Copy link
Contributor

@przemekwitek przemekwitek commented Feb 17, 2020

This PR makes the following indices hidden:

  1. .ml-anomalies*
  2. .ml-state*
  3. .ml-notifications*
  4. .ml-annotations*

Transform indices (.transform-notifications-* and deprecated .data-frame-notifications-*) and inference index (.ml-inference-000001) are not converted in this PR.

Relates #52420

@przemekwitek przemekwitek changed the title Make ml internal indices hidden [ML] Make ml internal indices hidden Feb 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@przemekwitek przemekwitek marked this pull request as ready for review February 18, 2020 12:49
@droberts195
Copy link
Contributor

I think line 941 of MachineLearning.java also needs deleting. At the time it was added it must have been expected that .ml-state would become a system index, not a hidden index. But since we want rollover type behaviour on it we've now decided that it will be a hidden index.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Do we need to worry about things like the ExpiredForecastsRemover?
It hits AnomalyDetectorsIndex.jobResultsIndexPrefix() + "*"; for its search but does not set the expansion flags to include hidden indices.

@przemekwitek
Copy link
Contributor Author

I think line 941 of MachineLearning.java also needs deleting. At the time it was added it must have been expected that .ml-state would become a system index, not a hidden index. But since we want rollover type behaviour on it we've now decided that it will be a hidden index.

Done.

Copy link
Contributor Author

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

Do we need to worry about things like the ExpiredForecastsRemover?
It hits AnomalyDetectorsIndex.jobResultsIndexPrefix() + "*"; for its search but does not set the expansion flags to include hidden indices.

My understanding is that it will work correctly as it provides the pattern starting with a dot ('.'). The problem occurs when (like in MlDistributedFailureIT.java) no index is provided.

@droberts195
Copy link
Contributor

It hits AnomalyDetectorsIndex.jobResultsIndexPrefix() + "*"; for its search but does not set the expansion flags to include hidden indices.

My understanding is that wildcard patterns still match hidden indices providing the pattern begins with a dot - see the second bullet point in the description of #50452. So we should be OK.

Once this PR is merged the whole team will need to be on the lookout for unforeseen side effects. It's good that we're not doing this close to feature freeze.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

👍 cool, glad hidden are still expanded if their prefix if specified.

@przemekwitek
Copy link
Contributor Author

run elasticsearch-ci/2

@przemekwitek
Copy link
Contributor Author

run elasticsearch-ci/2

@przemekwitek przemekwitek merged commit 0c309ef into elastic:master Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants