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] Job In Index: Enable GET APIS in mixed state #35344

Merged

Conversation

davidkyle
Copy link
Member

Jobs left open during an upgrade to 6.6 will not be migrated so this version must support running jobs that are stored in either the cluster state or index documents. New jobs will be stored as documents but old jobs will continue to run from the clusterstate. To achieve this the job and datafeed operations should look in both the clusterstate and the index for the configuration, this is the first change towards that goal and covers reading the configurations.

  • JobManager getJob/expandJob checks both cluster state and index
  • Added DatafeedConfigReader as a wrapper around DatafeedConfigProvider but checks both cluster state and index for configuration

All methods to get the configs go through these 2 classes.

This meant I had to change NameResolver to not throw on missing jobs as I now collect the clusterstate and index jobs and use ExpandedIdsMatcher to check the required job ids, datafeed ids and job groups are present.

Rolling upgrade tests to follow but this is already a large PR

@davidkyle davidkyle added >feature :ml Machine learning labels Nov 7, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@davidkyle davidkyle mentioned this pull request Nov 7, 2018
43 tasks
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.

Whew, pretty big PR. Found a couple of small things.

Will probably have to read through it all again to fully absorb it :D

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.

One last comment, and after clarification/correct, I think this looks good.

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.

LGTM, due to the size, do we want another 👍 ?

@davidkyle
Copy link
Member Author

I'm working on a follow up PR with much more rigorous testing in it which I didn't want to add to this one due to the size so I think we're ok with one review. I expect the upgrade tests I'm working on to flush out more bugs

@davidkyle davidkyle merged commit 64ba62a into elastic:feature-jindex-6x Nov 13, 2018
@davidkyle davidkyle deleted the enable-apis-in-mixed-state branch November 13, 2018 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :ml Machine learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants