Make sure that disabled jobs are paused in Quartz #25408

machi1990 · 2022-05-05T22:19:41Z

Disabled methods whose jobs details and triggers were already in the
database were still getting triggered because Quartz invokes all the
jobs in the database that are not in the paused State.
This is to make sure that the disabled methods are effectively not
triggered by Quartz in any way.

Furthermore, I do not believe the full scheduled method implementing the Application custom logic
was called because of the checks in the extensions which meant the
App business logic trigger was not full invoked but pausing the triggers
via Quartz we are sure that the trigger won't be invoked when it is
disabled.

Previously this was done via checks in the extension which mean that
only the Application logic of the scheduled method was not called.

quarkus/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

Lines 215 to 218 in 4df39b6

    
           OptionalLong everyMillis = SchedulerUtils.parseEveryAsMillis(scheduled); 
        
           if (!everyMillis.isPresent()) { 
        
               continue; 
        
           }

quarkus/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

Lines 168 to 170 in 4df39b6

if (SchedulerUtils.isOff(cron)) {

continue;

}

which meant that the trigger was not stored in known business triggers
at

quarkus/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

Lines 302 to 303 in 4df39b6

    
           scheduledTasks.put(identity, new QuartzTrigger(trigger, invoker, 
        
                   SchedulerUtils.parseOverdueGracePeriod(scheduled, defaultOverdueGracePeriod)));

meaning that upon the execution invocation of the method in

quarkus/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

Line 560 in 4df39b6

    
           public void execute(JobExecutionContext jobExecutionContext) throws JobExecutionException {

the business logic wasn't triggered but a NPE (same as #24931) could have been raised
with the code in main branch

machi1990 · 2022-05-05T22:21:40Z

extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

@@ -166,6 +166,7 @@ public QuartzScheduler(SchedulerContext context, QuartzSupport quartzSupport, Sc
                        String cron = SchedulerUtils.lookUpPropertyValue(scheduled.cron());
                        if (!cron.isEmpty()) {
                            if (SchedulerUtils.isOff(cron)) {
+                                this.pause(identity);


Let's pause the job disabled job to effectively let Quartz know that this should not be invoked in any way.
When the job is enabled/turned on, it'll be rescheduled and executed again.

This results in a org.quartz.Scheduler.pauseJob(JobKey) invocation, right? What happens if the job does not exist yet? Is it a no-op?

If the job does not exists, it is a no-op.
I'll add a test case in the integration test project to make sure we won't have a regression here.

machi1990 · 2022-05-05T22:21:53Z

extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

@@ -214,6 +215,7 @@ public QuartzScheduler(SchedulerContext context, QuartzSupport quartzSupport, Sc
                        } else if (!scheduled.every().isEmpty()) {
                            OptionalLong everyMillis = SchedulerUtils.parseEveryAsMillis(scheduled);
                            if (!everyMillis.isPresent()) {
+                                this.pause(identity);


Same comment as https://github.com/quarkusio/quarkus/pull/25408/files#r866356219

machi1990 · 2022-05-09T12:53:26Z

@ruiarodrigues this should fix the issue reported in #25214. Can you have a look?

ruiarodrigues · 2022-05-09T12:58:11Z

@machi1990 What am I suppose to do/check? 😄

machi1990 · 2022-05-09T13:18:34Z

@machi1990 What am I suppose to do/check? smile

:-)
I've tested that the patch effectively fixes the issue. I'll like a second eye on it as well, so it is possible, can you verify that this PR fixes the issue for you?

You can clone my fork: https://github.com/machi1990/quarkus and checkout to the fix/25214 branch.
Once that is done you can follow the instructions in https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#build (except the cloning the repo part) to build a local snapshot which you can use in the reproducer.
here is how you could use the snapshot: https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#usage

ruiarodrigues · 2022-05-09T14:18:55Z

I cloned it but I'm having some problems with the download of the dependencies. Is there any snapshot available of your fork?
I checked the PR and I have one question: the if(!everyMillis.isPresent()) statement already discard the job and doesn't add it to the scheduler or reschedule it. So why do you need to pause it here? Can it be read elsewhere and added to the scheduler?

machi1990 · 2022-05-09T14:34:13Z

I cloned it but I'm having some problems with the download of the dependencies. Is there any snapshot available of your fork?

No, I do not have a snapshot. What dependencies issue are you encountering?

I checked the PR and I have one question: the if(!everyMillis.isPresent()) statement already discard the job and doesn't add it to the scheduler or reschedule it. So why do you need to pause it here? Can it be read elsewhere and added to the scheduler?

This is explained in the PR description. The job is discarded and this is fine for in memory jobs but for persisted jobs quartz will try to launch them (

quarkus/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java

Line 563 in ace0e89

    
           public void execute(JobExecutionContext jobExecutionContext) throws JobExecutionException {

) - load them from the db. The way we wired the jobs in the current main codebase (it might be different from the version of quarkus the reproducer is running), the trigger method annotated with @Scheduled won't be executed - previously this would have cause a NPE (issue #24931, fixed by #25407) . With just that, the @scheduled trigger will be skipped all together.
Now the change in this PR really make sure that Quartz does not try to fire the trigger for disabled jobs. They'll be resumed automatically when the job is enabled / turned on again.

ruiarodrigues · 2022-05-09T15:02:16Z

From you explanation, the fix seems fine. I was able to compile finally. I let you know.

ruiarodrigues · 2022-05-09T18:03:55Z

I'm not able to run the reproducer with the 999-SNAPSHOT. I think you can go ahead and I will test it again when the final release is available

machi1990 · 2022-05-09T19:49:42Z

I'm not able to run the reproducer with the 999-SNAPSHOT. I think you can go ahead and I will test it again when the final release is available

Thanks for trying anyway. I wonder what's the issue you are facing?
In either way, @mkouba would you be able to re-review and approve/merge. I think once this is in, it can still be tested using main SNAPSHOT - https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#checking-an-issue-is-fixed-in-main

mkouba · 2022-05-11T13:01:20Z

integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledMethodResource.java

+    @Path("cron")
+    @Produces(MediaType.TEXT_PLAIN)
+    public String getCronCount() {
+        return disabledScheduledMethods.valueSetByCronScheduledMethod;


You shouldn't access a static variable like this...

I didn't give this much thought since it is a variable that's there to only show that it is not initialized.

mkouba · 2022-05-11T13:02:12Z

integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledScheduledMethods.java

+
+@ApplicationScoped
+public class DisabledScheduledMethods {
+    static String valueSetByCronScheduledMethod = "";


I think that we should make these variables volatile...

Same reasoning as #25408 (comment)

While the read access above is more a matter of style the fact that this field is not thread-safe is risky and may cause intermittent test failures or false positives.

I see your point. I've added the suggestion and added some comments to the methods

mkouba

Looks good. I've added two minor comments about the test though...

Fixes quarkusio#25214 Disabled methods whose jobs details and triggers were already in the database were still getting triggered because Quartz invokes all the jobs in the database that are not in the paused State. This is to make sure that the disabled methods are effectively not triggered by Quartz in any way. Furthermore, I do not believe the full scheduled method implementing the Application custom logic was called because of the checks in the extensions which meant the App business logic trigger was not full invoked but pausing the triggers via Quartz we are sure that the trigger won't be invoked when it is disabled. Previously this was done via checks in the extension which mean that only the Application logic of the scheduled method was not called. - https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L215-L218 - https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L168-L170 which meant that the trigger was not stored in known business triggers at https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L302-L303 meaning that upon the execution invocation of the method in https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L560 the business logic wasn't triggered but a NPE (same as quarkusio#24931) could have been raised with the code in main branch

machi1990 · 2022-05-11T13:45:24Z

Looks good. I've added two minor comments about the test though...

Thanks. Can you have a look?

machi1990 · 2022-05-11T14:47:15Z

Thanks @mkouba

machi1990 requested a review from mkouba May 5, 2022 22:19

quarkus-bot bot added the area/scheduler label May 5, 2022

machi1990 added the triage/backport? label May 5, 2022

machi1990 commented May 5, 2022

View reviewed changes

machi1990 mentioned this pull request May 5, 2022

Disabled schedulers are not disabled #25214

Closed

machi1990 force-pushed the fix/25214 branch from 8f00d70 to ace0e89 Compare May 6, 2022 08:26

mkouba reviewed May 11, 2022

View reviewed changes

machi1990 force-pushed the fix/25214 branch from ace0e89 to 52e83f1 Compare May 11, 2022 13:44

machi1990 requested a review from mkouba May 11, 2022 13:45

mkouba approved these changes May 11, 2022

View reviewed changes

machi1990 merged commit 70a6197 into quarkusio:main May 11, 2022

quarkus-bot bot added this to the 2.10 - main milestone May 11, 2022

quarkus-bot bot added the kind/bugfix label May 11, 2022

machi1990 deleted the fix/25214 branch May 11, 2022 14:47

gsmet modified the milestones: 2.10 - main, 2.9.1.Final May 12, 2022

gsmet removed the triage/backport? label May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure that disabled jobs are paused in Quartz #25408

Make sure that disabled jobs are paused in Quartz #25408

machi1990 commented May 5, 2022

machi1990 May 5, 2022

mkouba May 6, 2022

machi1990 May 6, 2022

machi1990 May 5, 2022

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

mkouba May 11, 2022

machi1990 May 11, 2022

mkouba May 11, 2022

machi1990 May 11, 2022

mkouba May 11, 2022

machi1990 May 11, 2022

mkouba left a comment

machi1990 commented May 11, 2022

machi1990 commented May 11, 2022

	OptionalLong everyMillis = SchedulerUtils.parseEveryAsMillis(scheduled);
	if (!everyMillis.isPresent()) {
	continue;
	}

	scheduledTasks.put(identity, new QuartzTrigger(trigger, invoker,
	SchedulerUtils.parseOverdueGracePeriod(scheduled, defaultOverdueGracePeriod)));

Make sure that disabled jobs are paused in Quartz #25408

Make sure that disabled jobs are paused in Quartz #25408

Conversation

machi1990 commented May 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

ruiarodrigues commented May 9, 2022

ruiarodrigues commented May 9, 2022

machi1990 commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkouba left a comment

Choose a reason for hiding this comment

machi1990 commented May 11, 2022

machi1990 commented May 11, 2022