-
Notifications
You must be signed in to change notification settings - Fork 94
Adding DeScheduled Job Information to Jobs REST API #803
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
Adding DeScheduled Job Information to Jobs REST API #803
Conversation
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
============================================
- Coverage 31.43% 30.24% -1.20%
+ Complexity 150 148 -2
============================================
Files 34 34
Lines 1527 1574 +47
Branches 137 145 +8
============================================
- Hits 480 476 -4
- Misses 1010 1060 +50
- Partials 37 38 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
LastExecutionTime and LastExpectedExecuitionTIme have been added to a disabledJobs. JSON return has been updated. |
} | ||
|
||
log.info("Descheduling jobId: {}", id); | ||
jobInfo.setDescheduled(true); |
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.
Any idea why the PR is showing lines 121-130 as added? I think these are the same on the main branch so I'm confused why GH is showing these as added.
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.
oh I see now, the removeDeScheduledJob
method was added.
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.
As much as possible, I would like to isolate the changes in ScheduledJobInfo to keep track of job status (enabled/disabled). If a job is fully deleted then we should delete it regardless of status, I don't think we should have multiple branches of logic to check.
} | ||
} | ||
|
||
this.disabledJobInfoMap.get(indexName).put(jobId, jobInfo); |
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.
Let's make sure that disabled jobs are removed from the map containing active jobs and vice-versa. We should have a test for both cases. Create job -> disable -> re-enable the job and call list jobs after both disabling and re-enabling. Ensure that the same job only appears in the response once and with expected status.
exception -> log.debug("Failed to delete lock", exception) | ||
) | ||
); | ||
} else if (this.scheduler.getDeScheduledJobIds(shardId.getIndexName()).contains(delete.id())) { |
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.
Oh I see, there is a bug when deleting a disabled job that it may not also delete corresponding lock?
Maybe we should combine this with the block above since the code within the block is duplicated?
Map<String, Object> jobDetails = new LinkedHashMap<>(); | ||
String jobType = indexToJobProvider.get(indexName).getJobType(); | ||
|
||
jobDetails.put("job_type", jobType); |
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.
We should push the logic up into each respective class to put the onus on the class being serialized. The job_parameter class already implements ToXContentObject
so we can use that for the parameter, but the JobSchedulingInfo
would need to be updated to specify how it should be serialized.
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.
This is an example of how you can rely on toXContent
when constructing a response: https://github.com/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java#L144-L146
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.
FYI I found that the code already handles disabled jobs in a couple of (seemingly) repetitive ways:
- The JobScheduler.schedule method aborts fast: https://github.com/opensearch-project/job-scheduler/blob/main/src/main/java/org/opensearch/jobscheduler/scheduler/JobScheduler.java#L74-L76
- JobScheduler.reschedule checks on each instance of a job running if its been descheduled and then doesn't schedule it for a run: https://github.com/opensearch-project/job-scheduler/blob/main/src/main/java/org/opensearch/jobscheduler/scheduler/JobScheduler.java#L183-L208
I think we can actually keep track of all jobs in the same data structure regardless of status as disabled jobs are already handled in many ways.
Signed-off-by: Jeremy Dupras <[email protected]>
Currently working to refactor how descheduled Jobs are stored |
Description
The Jobs REST API currently only lists the scheduled jobs which have the fields "descheduled = false and enabled = true". This will allow the user to see all jobs in job scheduler whether or not they are scheduled in both cluster wide call and by_node call.
There is no difference in the API call. The differences can be seen in the Descheduled and Enabled Fields. Additionally, "last_execution_time", "last_expected_execution_time", and "next_expected_execution_time" are all set to null values when the job is removed from the scheduled job info map. See the example JSON output below.
Lastly, when a job is deleted it is removed from both the scheduled job and the deScheduled jobs and will not show in the API call.
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.