Skip to content
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

Only start Quartz when there are some @Scheduled annotations #4554

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Only start Quartz when there are some @Scheduled annotations #4554

merged 1 commit into from
Oct 15, 2019

Conversation

SlyngDK
Copy link
Contributor

@SlyngDK SlyngDK commented Oct 14, 2019

…en there are some Schedules.

Fixes: #4243

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@gsmet gsmet changed the title [fixes quarkusio#4243] - Added check to ensure Quartz only started wh… Only start Quartz when there are some @Scheduled annotations Oct 15, 2019
@mkouba mkouba added this to the 0.25.0 milestone Oct 15, 2019
@gsmet
Copy link
Member

gsmet commented Oct 15, 2019

I rebased and force-pushed an adjusted message to try to get that one into 0.25.0.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 15, 2019
@jaikiran
Copy link
Member

jaikiran commented Oct 15, 2019

Unless we provide a way to schedule a job programatically...

@mkouba, Looking at the code, it looks like the io.quarkus.scheduler.Scheduler can be injected and can be used programatically to schedule jobs https://github.com/quarkusio/quarkus/blob/master/extensions/scheduler/runtime/src/main/java/io/quarkus/scheduler/Scheduler.java.

The implementation of these APIs, require the scheduler to be started before calling something like startTimer. So if we don't start scheduler, in the absence of a @Schedule, I think that would mean the scheduler can no longer be used programatically.

@gsmet
Copy link
Member

gsmet commented Oct 15, 2019

CI failures are unrelated, merging.

@gsmet gsmet merged commit 5b8f6a4 into quarkusio:master Oct 15, 2019
@gsmet
Copy link
Member

gsmet commented Oct 15, 2019

Thanks @SlyngDK !

@mkouba
Copy link
Contributor

mkouba commented Oct 16, 2019

@jaikiran I've completely forgotten about this one! In theory, we could detect Scheduler injection points too. In which case the only problematic scenario would be "programmatic lookup of a Scheduler + programmatic scheduling" combo.

@jaikiran
Copy link
Member

jaikiran commented Oct 16, 2019

In theory, we could detect Scheduler injection points too.

Yes, that's a good point.

In which case the only problematic scenario would be "programmatic lookup of a Scheduler + programmatic scheduling" combo.

So I had a look at what prompted this change and it appears to be this enhancement suggestion (a valid one IMO) - #4243. I think if that has to be solved and yet be able to start the scheduler eagerly, perhaps a better way would be to implement a (lazy?) org.quartz.spi.ThreadPool which doesn't start those threads eagerly.

@mkouba
Copy link
Contributor

mkouba commented Oct 16, 2019

@jaikiran Hm, maybe we should reuse the internal thread pool. Pls file a new tracking issue.

@jaikiran
Copy link
Member

@mkouba Done - #4606

@SlyngDK SlyngDK deleted the issue-4243 branch October 17, 2019 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scheduler triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quartz dependency shouldn't start threads when it's not being used
5 participants