-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[ML] Job In Index: Enable GET APIS in mixed state #35344
Conversation
This enables calls to the get job/stats APIs in a mixed cluster state before jobs have been migrated
Plus tests and fixes to expandJobs
All calls should use the calls that also check the cluster state
Pinging @elastic/ml-core |
There was a problem hiding this 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
...gin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetDatafeedsStatsAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedConfigReader.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java
Show resolved
Hide resolved
There was a problem hiding this 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.
...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetDatafeedsAction.java
Show resolved
Hide resolved
There was a problem hiding this 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 👍 ?
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 |
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.
DatafeedConfigReader
as a wrapper around DatafeedConfigProvider but checks both cluster state and index for configurationAll 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 useExpandedIdsMatcher
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