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

Support TimeUnit in the @Scheduled annotation #27309

Closed
wants to merge 11 commits into from
Closed

Support TimeUnit in the @Scheduled annotation #27309

wants to merge 11 commits into from

Conversation

vbrown37
Copy link
Contributor

Added an option to use java.util.concurrent.TimeUnit with fixedDelay, fixedRate and initialDelay in the @Scheduled annotation. Using by default milliseconds as before.

@pivotal-cla
Copy link

@Axzial Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@Axzial Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 21, 2021
@vbrown37
Using TimeUnit with @scheduled annotation
54e41f3
@vbrown37
Merge remote-tracking branch 'origin/main'
5310ec6 
# Conflicts:
#	spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java
@vbrown37
Using TimeUnit with @scheduled annotation
07d5ac3
@vbrown37
Merge remote-tracking branch 'origin/main'
2f55a7e 
# Conflicts:
#	spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java
@vbrown37
Merge remote-tracking branch 'origin/main'
23c6985
@vbrown37
Merge remote-tracking branch 'origin/main'
3ea9d21 
# Conflicts:
#	spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Aug 22, 2021
@sbrannen sbrannen changed the title Using TimeUnit with @Scheduled annotation Support TimeUnit in the @Scheduled annotation Aug 22, 2021
@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Aug 22, 2021
@sbrannen sbrannen removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 22, 2021
@sbrannen sbrannen added this to the 5.3.10 milestone Aug 23, 2021
sbrannen
sbrannen requested changes Aug 23, 2021
View reviewed changes
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

I've requested some minor changes.

Please also introduce some tests that use different TimeUnit values.

If you can get these changes in soon, this can be included in Spring Framework 5.3.10.

spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java Outdated Show resolved Hide resolved
spring-context/src/main/java/org/springframework/scheduling/annotation/Scheduled.java Outdated Show resolved Hide resolved
...ain/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java Outdated Show resolved Hide resolved
@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Aug 23, 2021
@sbrannen sbrannen self-assigned this Aug 23, 2021
sbrannen
sbrannen approved these changes Aug 23, 2021
View reviewed changes
Copy link
Member

@sbrannen sbrannen 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 making the requested changes!

I'll make a final review of this PR in time for 5.3.10.

@sbrannen sbrannen removed the status: waiting-for-feedback We need additional information before we can continue label Aug 23, 2021
sbrannen
sbrannen reviewed Aug 25, 2021
View reviewed changes
...ain/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java
@@ -407,7 +408,7 @@ protected void processScheduled(Scheduled scheduled, Method method, Object bean)
}
if (StringUtils.hasLength(initialDelayString)) {
try {
initialDelay = parseDelayAsLong(initialDelayString);
initialDelay = TimeUnit.MILLISECONDS.convert(parseDelayAsLong(initialDelayString), scheduled.timeUnit());
Copy link
Member

Choose a reason for hiding this comment

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

While performing a final review, I noticed that this introduces a 🐛 when the String is a java.time.Duration string, since the value gets converted to milliseconds and then converted again using the configured time unit.

I've fixed this locally, and you can see the result once I've merged the changes into main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, good to know ! Thank you.

@sbrannen
Copy link
Member

@Axzial, if you would like for your name to be included in the @author tags, please supply me your first name and last name.

@vbrown37
Copy link
Contributor Author

@Axzial, if you would like for your name to be included in the @author tags, please supply me your first name and last name.

Sure,

Victor Brown,

Thank you.

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Aug 25, 2021
@sbrannen
Revise and document TimeUnit support in @scheduled
bd72e44 
This commit also fixes a bug introduced in commit e99b43b, where
java.time.Duration strings were converted to milliseconds and then
converted again using the configured TimeUnit.

See spring-projectsgh-27309
@sbrannen sbrannen closed this in e99b43b Aug 25, 2021
@sbrannen
Copy link
Member

This has been merged into main in e99b43b and revised in bd72e44.

Thanks

sbrannen added a commit that referenced this pull request Aug 26, 2021
@sbrannen
Fix typo in @scheduled
9a7fb70 
See gh-27309
@sync-by-unito sync-by-unito bot mentioned this pull request Sep 15, 2021
Closed
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@vbrown37 @lxbzmy
Support TimeUnit in the @scheduled annotation
bbcb4b6 
This commit introduces a new `timeUnit` attribute in the @scheduled
annotation to allow the user to specify a time unit other than
milliseconds.

Closes spring-projectsgh-27309
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@sbrannen @lxbzmy
Revise and document TimeUnit support in @scheduled
3f7d017 
This commit also fixes a bug introduced in commit e99b43b, where
java.time.Duration strings were converted to milliseconds and then
converted again using the configured TimeUnit.

See spring-projectsgh-27309
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@sbrannen @lxbzmy
Fix typo in @scheduled
b94296f 
See spring-projectsgh-27309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrannen sbrannen sbrannen approved these changes

Assignees

@sbrannen sbrannen

Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Milestone
5.3.10
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vbrown37 @pivotal-cla @sbrannen @spring-projects-issues