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.

Sign up for GitHub

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

Jump to bottom

Consolidate scheduler extension - part 1 #5481

Merged
merged 2 commits into from
Nov 18, 2019
Merged

Consolidate scheduler extension - part 1 #5481

merged 2 commits into from
Nov 18, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Nov 14, 2019 •
edited by machi1990
Loading

  • quarkus-scheduler contains the API (io.quarkus.scheduler.Scheduled and friends), shared logic to collect/validate scheduled methods and a simple scheduler impl (we use com.cronutils:cron-utils to parse/validate/migrate CRON expressions, a ScheduledExecutorService with one "scheduler" thread that delegates the execution of scheduled methods to the ExecutorService obtained from ExecutorBuildItem; if quarkus-scheduler is present this impl is disabled)
  • quarkus-quartz depends on quarkus-scheduler and reuses most of the build steps and API; however, it's using Quartz as the underlying scheduler impl and provides some quartz-specific configuration
  • CRON syntax is configurable through the quarkus.scheduler.cron-type property; the default value is quartz to retain backward compatibility; other options are unix and cron4j
  • the Scheduler#startTimer() method was removed - it was a mistake
  • there is a new intergration test because previously scheduler was included in quarkus-integration-test-main but there were no assertions

TODO (next PRs):

@mkouba mkouba requested review from dmlloyd and machi1990 November 14, 2019 15:34
@mkouba mkouba mentioned this pull request Nov 14, 2019
Closed
@mkouba mkouba added this to the 1.1.0 milestone Nov 14, 2019
dmlloyd
dmlloyd approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

Otherwise seems fine to me.

@geoand
Copy link
Contributor

geoand commented Nov 15, 2019

The test failures seem related

@mkouba
Copy link
Contributor Author

mkouba commented Nov 15, 2019

@geoand OK, I'll investigate the failure.

@geoand
Copy link
Contributor

geoand commented Nov 15, 2019

Cool

@mkouba mkouba force-pushed the scheduler-next branch 2 times, most recently from b279c1f to b1feebb Compare November 15, 2019 11:15
@machi1990
Copy link
Member

Thanks @mkouba for this PR. I'll have a look at it tomorrow and revive my work on quartz clustered jobs.

@machi1990 machi1990 mentioned this pull request Nov 15, 2019
Closed
@mkouba
Copy link
Contributor Author

mkouba commented Nov 15, 2019

Ok, we're green now! ;-) Let's wait for @machi1990 's review though...

machi1990
machi1990 approved these changes Nov 15, 2019
View reviewed changes
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

Added minor comments, otherwise LGTM. Great progress :-)

I have already started reviving #3783 based on this work.

@machi1990 machi1990 mentioned this pull request Nov 16, 2019
Merged
@mkouba
Consolidate scheduler extension - part 1
54dfc3b 
- introduce lightweight scheduler implementation in quarkus-scheduler
- introduce quarkus-quartz based on quarkus-scheduler build steps and API
- remove Scheduler#startTimer() method
- add proper intergration test
@mkouba
Scheduler - do not use interceptor to activate request context per exec
92f71a4 
- otherwise the scheduled bean must be proxyable
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 18, 2019
@machi1990
Copy link
Member

@mkouba CI is green, we can merge this right? I would like to pick up some changes you made in my PR #5529

@mkouba mkouba merged commit a62522d into quarkusio:master Nov 18, 2019
@machi1990
Copy link
Member

You rock @mkouba ;-)

machi1990 added a commit to machi1990/quarkus that referenced this pull request Nov 18, 2019
@machi1990
docs: mention the quarkus.scheduler.cron-type property
f06799e 
Follows up quarkusio#5481
@machi1990 machi1990 mentioned this pull request Nov 18, 2019
Merged
@mkouba mkouba mentioned this pull request Nov 18, 2019
Closed
machi1990 added a commit to machi1990/quarkus that referenced this pull request Nov 18, 2019
@machi1990
docs: mention the quarkus.scheduler.cron-type property
6bfc18d 
Follows up quarkusio#5481
@machi1990 machi1990 mentioned this pull request Nov 19, 2019
Closed
ia3andy pushed a commit to dmlloyd/quarkus that referenced this pull request Nov 19, 2019
@machi1990 @ia3andy
docs: mention the quarkus.scheduler.cron-type property
0da8727 
Follows up quarkusio#5481
Simulant87 pushed a commit to Simulant87/quarkus that referenced this pull request Nov 23, 2019
@machi1990 @Simulant87
docs: mention the quarkus.scheduler.cron-type property
fb7a448 
Follows up quarkusio#5481
Simulant87 pushed a commit to Simulant87/quarkus that referenced this pull request Nov 23, 2019
@machi1990 @Simulant87
docs: mention the quarkus.scheduler.cron-type property
34872ba 
Follows up quarkusio#5481
mmusgrov pushed a commit to mmusgrov/quarkus that referenced this pull request Dec 13, 2019
@machi1990 @mmusgrov
docs: mention the quarkus.scheduler.cron-type property
0c894af 
Follows up quarkusio#5481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machi1990 machi1990 machi1990 approved these changes

@dmlloyd dmlloyd dmlloyd approved these changes

Assignees
No one assigned
Labels
release/breaking-change triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.1.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@mkouba @geoand @machi1990 @dmlloyd