-
Notifications
You must be signed in to change notification settings - Fork 40.8k
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
Add the ability to trigger a Quartz job on-demand through an Actuator endpoint #43086
base: main
Are you sure you want to change the base?
Conversation
c20c53d
to
8dbd473
Compare
@@ -298,7 +298,7 @@ protected interface LinksHandler { | |||
@FunctionalInterface | |||
protected interface ReactiveWebOperation { | |||
|
|||
Mono<ResponseEntity<Object>> handle(ServerWebExchange exchange, Map<String, String> body); | |||
Mono<ResponseEntity<Object>> handle(ServerWebExchange exchange, Map<String, Object> body); |
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 you explain why you had to change this?
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 added a comment regarding these changes in the #42530 (comment)
The Map<String, String> request body does not support complex values like Map or List, only basic JSON types.
For example, this JSON is not supported at the moment:
{
"jobData": {
"key": "value"
}
}
WebMvc:
[org.springframework.http.converter.HttpMessageNotReadableException:
JSON parse error: Cannot deserialize value of type `java.lang.String` from Object value (token `JsonToken.START_OBJECT`)]
WebFlux:
org.springframework.core.codec.DecodingException: JSON decoding error: Cannot deserialize value of type `java.lang.String` from Object value (token `JsonToken.START_OBJECT`)
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.
Unfortunately, we don't intend to support arbitrary input this way. This part is covered in the reference guide.
Can you please revert to allow jobs to be invoked on demand without custom data?
In the comment you've referenced you stated that "By the way, Jersey works fine". I am not sure what you mean by that but if there is something that needs to be done for things to work currently the same way on all stacks, please create a separate issue with more details.
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 meant that @WebEndpointTest
with JERSEY
infrastructure does support Map<String, Object
as an endpoint parameter.
If I revert my changes related to @RequestBody Map<String, Object
the MVC and WebFlux will fail but Jersey will not.
You can reproduce this issue by running this test QuartzEndpointWebIntegrationTests.quartzTriggerJobWithCustomData
Branch: https://github.com/nosan/spring-boot/tree/42530-map-issue
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.
709aa31
to
084bd7f
Compare
Thanks, @snicoll |
@@ -79,6 +80,18 @@ public WebEndpointResponse<Object> quartzJobOrTrigger(SecurityContext securityCo | |||
() -> this.delegate.quartzTrigger(group, name, showUnsanitized)); | |||
} | |||
|
|||
@WriteOperation | |||
public WebEndpointResponse<Object> triggerQuartzJob(@Selector String jobs, @Selector String group, |
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're not fan of this approach and we don't want the action to be part of the URL. We've brainstormed quite a bit and we believe something else is missing that I am going to tackle in #43226.
Rather than this, we'd like an approach where a POST
on the job detail could trigger a new execution if the payload contains "running" : true
. Can you please review the PR in that direction? It doesn't need the related issue but we'll process them both once they're ready to be merged.
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.
Thanks, @snicoll.
Initially, I was thinking of including action=TRIGGER|RUN|EXECUTE
as part of the request body.
I have one more question regarding running:true
. What should be returned if someone provides running:false
? Should it result in a bad request or a not-found response?
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 was thinking about action
because quartz supports pause
, resume
actions a well.
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 feature might be requested in the future. For instance, if a job is broken or encountering issues, someone might want the ability to pause it.
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 idea is that you POST
a body indicating the desired state. For a job, posting running: true
indicates that the job should be run now. AFAIK, Quartz doesn't support cancellation of a running job so posting running:false
would be a bad request.
AFAIK, pausing is supported by triggers not jobs so I'd expect that support to be added to a resource that represents a trigger. You'd post something like "state": "paused"
to pause a trigger and "state": "normal"
to resume it.
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.
AFAIK, pausing is supported by triggers not jobs so I'd expect that support to be added to a resource that represents a trigger. You'd post something like "state": "paused" to pause a trigger and "state": "normal" to resume it.
/**
* Pause the <code>{@link org.quartz.JobDetail}</code> with the given
* key - by pausing all of its current <code>Trigger</code>s.
*
* @see #resumeJob(JobKey)
*/
void pauseJob(JobKey jobKey)
throws SchedulerException;
/**
* Resume (un-pause) the <code>{@link org.quartz.JobDetail}</code> with
* the given key.
*
* <p>
* If any of the <code>Job</code>'s<code>Trigger</code> s missed one
* or more fire-times, then the <code>Trigger</code>'s misfire
* instruction will be applied.
* </p>
*
* @see #pauseJob(JobKey)
*/
void resumeJob(JobKey jobKey)
throws SchedulerException;
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.
Thanks, @wilkinsona
just to clarify one more time there should be POST /actuator/quartz/jobs/{groupName}/{jobName}
with the JSON body
{
"running": 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.
Thanks. I'd missed the API for pausing and resuming a job. It's an alias for pausing/resuming all of a job's triggers and doesn't affect the state of the job itself, just its associated triggers. I'm not sure how we might add similar functionality to the Quartz endpoint. Posting something like state: paused
to a job doesn't feel right to me given that it's actually the triggers' state that would change. It's also not clear to me what a job's state would be if some of its triggers were paused and others were running as normal.
just to clarify one more time…
Yes, that's the preferred direction right now. I'm not totally sure about "running": "true"
vs something like "state": "running"
. The latter feels like it would provide more scope for changes in the future but it should be relatively easy to change the exact payload once the general structure's 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.
To be honest, I do like "state": "running"
more than "running": "true"
Thank you, @wilkinsona and @snicoll The PR has been updated. I've implemented the |
… endpoint Before this commit, triggering a Quartz job on demand was not possible. This commit introduces a new @WriteOperation endpoint at /actuator/quartz/jobs/{groupName}/{jobName}, allowing a job to be triggered by specifying the jobName and groupName See spring-projectsgh-42530
Before this commit, triggering a Quartz job on demand was not possible. This commit introduces a new
@WriteOperation
endpoint at/actuator/quartz/jobs/{groupName}/{jobName}/trigger
, allowing a job to be triggered by specifying the{jobName}
,{groupName}
, and an optionalJobDataMap
.See gh-42530