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

Manage Quartz Jobs through ArC #11507

Merged
merged 2 commits into from
Aug 22, 2020
Merged

Manage Quartz Jobs through ArC #11507

merged 2 commits into from
Aug 22, 2020

Conversation

gastaldi
Copy link
Contributor

Fixes #11496

@gastaldi gastaldi requested a review from manovotn August 21, 2020 01:21
@gastaldi gastaldi changed the title Manage Jobs through ArC Manage Quartz Jobs through ArC Aug 21, 2020
Copy link
Contributor

@manovotn manovotn 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!

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.

Can we add a line or two in the Quartz documentation?

Other than that it LGTM too.

Thank you!

@gastaldi gastaldi marked this pull request as draft August 21, 2020 14:49
@gastaldi
Copy link
Contributor Author

Found a small bug while running the example in the docs. Let me double check before marking this PR for review

@gastaldi gastaldi marked this pull request as ready for review August 21, 2020 16:31
@gastaldi gastaldi added this to the 1.8.0 - master milestone Aug 21, 2020
@gastaldi
Copy link
Contributor Author

Should be good now, sorry for the trouble 😉

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.

Thanks for the followup fixes!

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 21, 2020
@gastaldi gastaldi merged commit 75aa0b6 into quarkusio:master Aug 22, 2020
@gastaldi gastaldi deleted the quartz_cdi branch August 22, 2020 01:36
@mkouba
Copy link
Contributor

mkouba commented Aug 24, 2020

@gastaldi @machi1990 I think that a better and more correct solution would be to inject Instance<Job> into QuartzScheduler and use it in InvokerJobFactory. That way all beans implementing Job would be unremovable automatically and no AdditionalBeanBuildItem would be needed (note that AdditionalBeanBuildItem should be only used to add a bean class that is not in any bean archive).

@machi1990
Copy link
Member

@gastaldi @machi1990 I think that a better and more correct solution would be to inject Instance<Job> into QuartzScheduler and use it in InvokerJobFactory. That way all beans implementing Job would be unremovable automatically and no AdditionalBeanBuildItem would be needed (note that AdditionalBeanBuildItem should be only used to add a bean class that is not in any bean archive).

Welcome back and thanks for the input / note @mkouba.

Happy to review any followup PR if @gastaldi or anyone will be up for it :-)

@mkouba
Copy link
Contributor

mkouba commented Aug 24, 2020

I can try to prepare a followup PR today...

@machi1990
Copy link
Member

Thanks @mkouba

mkouba added a commit to mkouba/quarkus that referenced this pull request Aug 24, 2020
gsmet pushed a commit to mkouba/quarkus that referenced this pull request Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a cdi aware QuartzFactory
4 participants