michaelklishin / quartz-mongodb

A MongoDB-based store for the Quartz scheduler. This fork strives to be as feature complete as possible. Originally by MuleSoft.
Other
249 stars 203 forks source link

JobExecutionContext.getPreviousFireTime returns actual execution time #6

Closed daniel-h closed 11 years ago

daniel-h commented 11 years ago

When getting the previous fire time in a Job implementation via JobExecutionContext.getPreviousFireTime() it actually returns the current time of execution.

It seems the previousFireTime is set to early in the process.

I would assume it should be set after the job has been completed, when the lock is removed, taking the time from the lock entry as previousFireTime?

Best regards, Daniel

michaelklishin commented 11 years ago

Feel free to submit a pull request if you think you understand what the problem is and what's the right way to solve it :) Thank you!

daniel-h commented 11 years ago

After some debugging it seems the issue is as follows:

In method MongoDBJobStore.triggersFired()

The code is updating the trigger (and thus trigger times) trigger.triggered(cal); storeTrigger(trigger, true);

before passing the trigger values to the TriggerFiredBundle which in turn provide it in the JobExecutionContext.

The fix is to move the two lines of code to the end of the for-loop so it reads:

results.add(new TriggerFiredResult(bndle)); // Now update the trigger trigger.triggered(cal); storeTrigger(trigger, true);

I have tested it successfully, but please review and confirm the fix, since I don't know all the details of Quartz

Best regards, Daniel

michaelklishin commented 11 years ago

@ifesdjeen: please take a look

daniel-h commented 11 years ago

Submitted pull request now. Hope I did it right, was the first time :-)

ifesdjeen commented 11 years ago

Checked out PR, looked @ JobStoreSupport code.

        Date prevFireTime = trigger.getPreviousFireTime();

        // call triggered - to update the trigger's next-fire-time state...
        trigger.triggered(cal);

I would say I see the correlation, too...

qtxo commented 5 years ago

I was bitten by this problem in 2.1.0, tldr DON'T use trigger.getPreviousFireTime() use: JobExecutionContext.getPreviousFireTime()