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

Fix SimpleScheduler pause functionality #13532

Merged
merged 1 commit into from
Nov 28, 2020
Merged

Fix SimpleScheduler pause functionality #13532

merged 1 commit into from
Nov 28, 2020

Conversation

sultansoy
Copy link
Contributor

@sultansoy sultansoy commented Nov 27, 2020 •
edited
Loading

Fix #13512
Add return in SimpleScheduler::checkTriggers method if !running.

@ghost
Copy link

ghost commented Nov 27, 2020

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.

@ghost ghost added the area/scheduler label Nov 27, 2020
@sultansoy sultansoy changed the title Fix SimpleScheduler pause functionality #13512 Fix SimpleScheduler pause functionality Nov 27, 2020
machi1990
machi1990 requested changes Nov 28, 2020
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.

Good catch and thank you for the PR @sultansoy. Do you think we can add a test for this?

We can take inspiration from https://github.com/quarkusio/quarkus/blob/04842681f5f46f3ef4be84921838a2a10bf22ecf/extensions/scheduler/deployment/src/test/java/io/quarkus/scheduler/test/DisabledSchedulerTest.java by enabling quarkus.scheduler.enabled=true and pausing the scheduler and check that no scheduled method was called.

@sultansoy sultansoy force-pushed the fix-simple-scheduler branch 2 times, most recently from 45c8c1e to cbcf737 Compare November 28, 2020 10:36
gsmet
gsmet previously requested changes Nov 28, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Just a quick note that this needs to be squashed before merging. Please dismiss when done.

@sultansoy sultansoy requested a review from machi1990 November 28, 2020 12:15
@sultansoy @RustamSultansoy
Fix SimpleScheduler pause functionality.
5bbc400 
Add return in SimpleScheduler::checkTriggers method if !running.

Unit test for SimpleScheduler::pause functionality
Pause and check isRunning returns false and scheduler doesn't trigger CountDownLatch::countDown
@sultansoy sultansoy force-pushed the fix-simple-scheduler branch from 63ed5da to 5bbc400 Compare November 28, 2020 13:29
@machi1990
Copy link
Member

Thanks @sultansoy. LGTM

@machi1990 machi1990 added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 28, 2020
@sultansoy
Copy link
Contributor Author

Just a quick note that this needs to be squashed before merging. Please dismiss when done.

squashed

@sultansoy sultansoy requested a review from gsmet November 28, 2020 13:41
gsmet
gsmet approved these changes Nov 28, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@sultansoy
Copy link
Contributor Author

@machi1990 @gsmet sorry, should I retrigger the CI?

@gsmet gsmet merged commit 47e80c5 into quarkusio:master Nov 28, 2020
@ghost ghost added this to the 1.11 - master milestone Nov 28, 2020
@gsmet
Copy link
Member

gsmet commented Nov 28, 2020

No, it's OK, merged, thanks!

@gsmet gsmet modified the milestones: 1.11 - master, 1.10.2.Final Nov 30, 2020
@gsmet gsmet modified the milestones: 1.10.2.Final, 1.7.6.Final Dec 11, 2020
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

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/scheduler triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.7.6.Final
Development

Successfully merging this pull request may close these issues.

SimpleScheduler: pause doesn't work
3 participants
@sultansoy @machi1990 @gsmet