-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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] JIndex: Job exists and get job should read cluster state first. #36305
[ML] JIndex: Job exists and get job should read cluster state first. #36305
Conversation
Pinging @elastic/ml-core |
@@ -366,6 +341,13 @@ public void putJob(PutJobAction.Request request, AnalysisRegistry analysisRegist | |||
return; | |||
} | |||
|
|||
// Check the job id is not the same as a group Id | |||
if (currentMlMetadata.isGroupOrJob(job.getId())) { |
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.
Are we also validating this when the job is written to the index?
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.
There are 2 checks here
- that the job id is not a group
- that any groups the job has are not job Ids
I realised the second was missing so I pushed another commit with that. Previously those checks were done in the constructor of MlMetadata
which further made me realise I need to perform the same check on job update so I made that change also.
d6f7b9b
to
f63f964
Compare
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
retest this please |
Also check the job Id is not a clusterstate group ID
f63f964
to
263ab23
Compare
Feature branch PR
Changing methods missed in #36014 - get job and job exists.
This also adds in a missing check on PUT job that the job Id is not the same as a group Id defined in clusterstate jobs and a another check on job update that the jobs groups are not job ids.