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

Quartz + Scheduler: Allow to use config in Identity field #14993

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Quartz + Scheduler: Allow to use config in Identity field #14993

merged 1 commit into from
Feb 15, 2021
+342 −63

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Feb 11, 2021 •
edited
Loading

These changes allow to use config properties in the identity field:

@Scheduled(identity = "{job.id}")

Fix #14967

@ghost
Copy link

ghost commented Feb 11, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

This message is automatically generated by a bot.

Sgitario
Sgitario commented Feb 11, 2021
View reviewed changes
...nsions/scheduler/deployment/src/test/java/io/quarkus/scheduler/test/PausedSchedulerTest.java Outdated
LATCH.countDown();
if (IS_WATCHING.get()) {
LATCH.countDown();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated to my changes, but it was randomly failing for me. Using the is watching boolean, made the test more consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I've never seen this error before. Do you mind putting this in a separate commit / PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've deleted my changes here and let's see if the CI complains. If so, I will create a separate PR for this.
Thanks!

@Sgitario Sgitario changed the title Quartz + Scheduler: Allow to use config in Identity field. Fixes #14967 Quartz + Scheduler: Allow to use config in Identity field Feb 11, 2021
@Sgitario
Copy link
Contributor Author

@machi1990 These changes allow to use confg values in the identity field which is important for Quartz.
However, I noticed that the test cases are almost duplicated in the scheduler and quartz extensions. To continue doing the same, I added the same test cases in both too, but let me know if this is not correct.

@gsmet gsmet requested review from mkouba and machi1990 February 11, 2021 11:25
machi1990
machi1990 reviewed Feb 12, 2021
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.

@Sgitario thanks for the quick PR.

LGTM, I've added some comments. Can you have a look?

@Sgitario Sgitario requested a review from machi1990 February 12, 2021 13:11
machi1990
machi1990 approved these changes Feb 12, 2021
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.

LGTM.
@mkouba can you have a look too?

mkouba
mkouba reviewed Feb 12, 2021
View reviewed changes
extensions/scheduler/runtime/src/main/java/io/quarkus/scheduler/runtime/SimpleScheduler.java Outdated Show resolved Hide resolved
extensions/scheduler/runtime/src/main/java/io/quarkus/scheduler/runtime/SimpleScheduler.java
@@ -188,13 +185,8 @@ SimpleTrigger createTrigger(String invokerClass, CronParser parser, Scheduled sc
}
}

// Keep it public so that we can reuse the logic in the quartz extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should extract these two static methods to a dedicated class? I'm not sure about the name though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the same approach in other extensions, I've created a SchedulerUtils and moved these two methods and also some static methods that were part of the interface SchedulerContext and were also related to configuration.

I left these methods as static but we could make them instance methods.

Let me know if it looks better now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current version looks perfect! Thanks.

@Sgitario Sgitario requested a review from mkouba February 15, 2021 06:46
@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label Feb 15, 2021
@machi1990 machi1990 merged commit 73ce177 into quarkusio:master Feb 15, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Feb 15, 2021
@Sgitario Sgitario deleted the quartz_identity branch February 15, 2021 12:06
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

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/documentation area/scheduler area/spring Issues relating to the Spring integration
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.

Quartz: Allow to use a configuration property in identity
3 participants
@Sgitario @mkouba @machi1990