-
Notifications
You must be signed in to change notification settings - Fork 94
Adds REST API to list jobs with an option to list them per node #786
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
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (33.76%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
============================================
- Coverage 37.67% 33.76% -3.92%
- Complexity 135 143 +8
============================================
Files 22 29 +7
Lines 1189 1380 +191
Branches 109 132 +23
============================================
+ Hits 448 466 +18
- Misses 704 877 +173
Partials 37 37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
@Override | ||
public List<Route> routes() { | ||
return List.of(new Route(GET, JobSchedulerPlugin.JS_BASE_URI + "/info")); |
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.
wdyt about renaming the route to GET /_plugins/_job_scheduler/api/jobs
?
I could imagine that we would also re-use this Rest action in the future to get info for a particular job.
i.e. GET /_plugins/_job_scheduler/api/jobs/{jobID}
Using that pattern would make JS inline with how the security plugin defines rest APIs: https://docs.opensearch.org/docs/latest/security/access-control/api
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.
+1. should be /_plugins/_job_scheduler/api/jobs/{jobID}
.
/_plugins/_job_scheduler/api/jobs?by_node
if by_node query-param is present then list by node else all jobs. No need for by_node=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.
Api call has been changed
|
||
try { | ||
// Create a list to hold all job details | ||
List<Map<String, Object>> jobs = new java.util.ArrayList<>(); |
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.
nit: can we use an import to avoid using the fully qualified class name here?
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.
imports added
String jobId = jobEntry.getKey(); | ||
JobSchedulingInfo jobInfo = jobEntry.getValue(); | ||
|
||
if (jobInfo == null) continue; |
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.
Can this scenario actually happen where a jobID exists without jobInfo? (job deleted?) If this can happen should we consider having a warning log?
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.
warning log added
// Get job provider type if available | ||
String jobType = "unknown"; | ||
|
||
if (indexToJobProvider.get(indexName).getJobType() != null) { |
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.
Similar as above. Are there instances where the index is not in the indexToJobProvider map? That sounds like an error/warn condition that we should log if it can happen
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.
The null check has been removed. Index to job provider is required for the initial scheduling of a job.
jobDetails.put("last_update_time", jobInfo.getJobParameter().getLastUpdateTime().toString()); | ||
// Add execution information | ||
|
||
if (jobInfo.getActualPreviousExecutionTime() != null) { |
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.
Isn't null valid for a value here if the job has never run before? i.e. I setup a report and schedule it to first run for tomorrow. I think we should still return this value in the response even if it is null
.
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 will now be added as "none" if the field is null.
jobDetails.put("index_name", indexName); | ||
|
||
// Add job parameter details | ||
if (jobInfo.getJobParameter() != null) { |
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.
job parameter is required so we shouldn't need a null check here
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.
Null check removed.
if (jobInfo.getActualPreviousExecutionTime() != null) { | ||
jobDetails.put("last_execution_time", jobInfo.getActualPreviousExecutionTime()); | ||
} | ||
if (jobInfo.getExpectedPreviousExecutionTime() != null) { |
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.
Same comments as above, let's include these in the response even if the values are not defined
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.
+1. Both info should be included even if they haven't run
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.
Field will be shown as "none".
jobDetails.put("next_expected_execution_time", jobInfo.getExpectedExecutionTime().toString()); | ||
} | ||
|
||
// Add next time to execute |
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.
Isn't this a repeat of the value above?
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.
Redundancy removed.
); | ||
|
||
// Add schedule information | ||
if (jobInfo.getJobParameter().getSchedule() != null) { |
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 should never be null. If it is let's make sure to have a warning/error log.
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.
error log added
src/test/java/org/opensearch/jobscheduler/multinode/GetScheduledInfoMultiNodeTransportIT.java
Show resolved
Hide resolved
src/.idea/workspace.xml
Outdated
@@ -0,0 +1,59 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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 remove this file
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.
Should be removed now.
// Refresh indices to ensure all jobs are available | ||
makeRequest(client(), "POST", "/_refresh", Collections.emptyMap(), null); | ||
|
||
Thread.sleep(1000); |
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.
Is the sleep required if we are explicitly calling refresh above?
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.
i would suggest using Awaitility.await if you need to wait for certain info to be available before proceeding with the tests
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.
Sleep function call has been removed.
...-plugin/src/test/java/org/opensearch/jobscheduler/sampleextension/GetScheduledJobInfoIT.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.
Thank you for this PR @Jeremydupras! This is a great start and goes a long way towards making the job scheduler more transparent and useful.
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.
The changes look good and are definitely in the right direction. I have left some suggestions and comments to improve testing.
I also suggest to update PR title to be more descriptive. Something like:
Adds REST API to list jobs with an option to list them per node
src/.idea/workspace.xml
Outdated
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 file probably was an accidental push and should be deleted.
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.
Should be fixed.
|
||
@Override | ||
public List<Route> routes() { | ||
return List.of(new Route(GET, JobSchedulerPlugin.JS_BASE_URI + "/info")); |
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.
+1. should be /_plugins/_job_scheduler/api/jobs/{jobID}
.
/_plugins/_job_scheduler/api/jobs?by_node
if by_node query-param is present then list by node else all jobs. No need for by_node=true
import org.opensearch.jobscheduler.transport.response.GetScheduledInfoResponse; | ||
|
||
public class GetScheduledInfoAction extends ActionType<GetScheduledInfoResponse> { | ||
public static final String NAME = "cluster:admin/opensearch/_job_scheduler/info"; |
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.
nit: cluster:admin/opensearch/job_scheduler/info
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.
changed to match the new api call
if (jobInfo.getActualPreviousExecutionTime() != null) { | ||
jobDetails.put("last_execution_time", jobInfo.getActualPreviousExecutionTime()); | ||
} | ||
if (jobInfo.getExpectedPreviousExecutionTime() != null) { |
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.
+1. Both info should be included even if they haven't run
|
||
// Add next time to execute | ||
java.time.Instant now = java.time.Instant.now(); | ||
jobDetails.put( |
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.
seems like a duplicate of above.
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.
Duplicate has been removed.
scheduleMap.put("interval", intervalSchedule.getInterval()); | ||
scheduleMap.put("unit", intervalSchedule.getUnit().toString()); | ||
} else if (jobInfo.getJobParameter().getSchedule() instanceof CronSchedule) { | ||
scheduleMap.put("type", "cron"); |
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 replace the values with constants declared in the classes, scheduleMap.put("type", "cron")
-> scheduleMap.put("type", CronSchedule.CRON_FIELD);
. Same for IntervalSchedule above.
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.
Tried to implement this and it was causing the program to crash. I will continue to look into having this implemented.
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.
Found a work around. I needed to change the permissions to the constants.
// Refresh indices to ensure all jobs are available | ||
makeRequest(client(), "POST", "/_refresh", Collections.emptyMap(), null); | ||
|
||
Thread.sleep(1000); |
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.
i would suggest using Awaitility.await if you need to wait for certain info to be available before proceeding with the tests
...-plugin/src/test/java/org/opensearch/jobscheduler/sampleextension/GetScheduledJobInfoIT.java
Show resolved
Hide resolved
@@ -1,6 +0,0 @@ | |||
<component name="CopyrightManager"> |
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.
These 2 files can remain. I know they are IDE specific files in a codebase that can be run in any editor, but its common to pull this repo into Intellij and these files ensure that copywrite headers are in place.
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.
Replaced files
import org.opensearch.jobscheduler.transport.response.GetScheduledInfoResponse; | ||
|
||
public class GetScheduledInfoAction extends ActionType<GetScheduledInfoResponse> { | ||
public static final String NAME = "cluster:admin/opensearch/_job_scheduler/api/jobs"; |
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.
public static final String NAME = "cluster:admin/opensearch/_job_scheduler/api/jobs"; | |
public static final String NAME = "cluster:admin/opensearch/jobscheduler/jobs/all"; |
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.
Changed
Object jobs = nodeResponse.getScheduledJobInfo().get("jobs"); | ||
if (jobs instanceof List) { | ||
for (Object job : (List<?>) jobs) { | ||
if (uniqueJobs.add(job)) { |
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 too generic here and I think it will add duplicates because its comparing generic Objects from 2 different GetScheduledInfoNodeResponse instead of checking based on job_id
.
Instead of Object can this be cast to the more narrow Map<String, Object>
or a separate class that contains all of the fields in the API response (like jobInfo or something similar?)
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.
Changed the logic to look at job ID's.
* remove guava dependency Signed-off-by: Jing Zhang <[email protected]> * remove more unused dependencies Signed-off-by: Jing Zhang <[email protected]> --------- Signed-off-by: Jing Zhang <[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]>
* Add a CHANGELOG and changelog_verifier workflow Signed-off-by: Craig Perkins <[email protected]> * spotless Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]> Signed-off-by: Jeremy Dupras <[email protected]>
…pensearch-project#780) Signed-off-by: Zelin Hao <[email protected]> Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Craig Perkins <[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: Jeremydupras <[email protected]>
@@ -0,0 +1,6 @@ | |||
<component name="CopyrightManager"> |
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 needs to be at the top level under .idea
and not under src
Signed-off-by: Jeremy Dupras <[email protected]>
Description
This Transport API gathers and returns all scheduled jobs within the Job Scheduler plugin. The resulting jobs can be displayed either by node or as a total list.
API Call -> GET /_plugins/_job_scheduler/api/jobs
-> GET /_plugins/_job_scheduler/api/jobs?by_node
Related Issues
Resolves #775
Does not include an extensive history of jobs.
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.