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 Kotlin suspend functions as scheduler methods #24741

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Support Kotlin suspend functions as scheduler methods #24741

merged 1 commit into from
Apr 8, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 4, 2022 •
edited
Loading

I had to juggle around some of the classes and introduce new modules in order to introduce Kotlin support in the same way we have done it for reactive messaging and in order to avoid split package issues.

Resolves: #21428

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/scheduler area/spring Issues relating to the Spring integration labels Apr 4, 2022
@geoand
Copy link
Contributor Author

geoand commented Apr 4, 2022

This is still in draft because I need to add some test and potentially clean up the code a bit.

@geoand geoand mentioned this pull request Apr 4, 2022
Merged
@geoand geoand force-pushed the #21428 branch 2 times, most recently from 12fd210 to c26a651 Compare April 4, 2022 17:10
@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2022

I had to juggle around some of the classes and introduce new modules in order to introduce Kotlin support in the same way we have done it for reactive messaging and in order to avoid split package issues.

Hm, can't quarkus-scheduler-kotlin just depend on quarkus-scheduler? It's not even an extension, or? I don't understand why the common and api modules are needed?

@geoand
Copy link
Contributor Author

geoand commented Apr 5, 2022

Hm, can't quarkus-scheduler-kotlin just depend on quarkus-scheduler

It can, but that means that users will have to pull it in explicitly.
The way it's done now, all users have to do is include quarkus-kotlin and quarkus-scheduler

@geoand geoand force-pushed the #21428 branch 2 times, most recently from c62e382 to 1f4a028 Compare April 5, 2022 06:58
@geoand geoand marked this pull request as ready for review April 5, 2022 06:58
@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2022

Hm, can't quarkus-scheduler-kotlin just depend on quarkus-scheduler

It can, but that means that users will have to pull it in explicitly. The way it's done now, all users have to do is include quarkus-kotlin and quarkus-scheduler

I see. What magic does include the relevant extensions? ;-)

@geoand
Copy link
Contributor Author

geoand commented Apr 5, 2022

In this case there is no magic, it's just that quarkus-scheduler brings in quarkus-scheduler-kotlin, but that one has optional dependencies on Kotlin, which are basically met when quarkus-kotlin is included.

We have used the same exact strategy in Kotlin support for Reactive Messaging. In that case we didn't need to introduce these api and common modules split, because the API is in RM and could be references by without changes.

@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2022

In this case there is no magic, it's just that quarkus-scheduler brings in quarkus-scheduler-kotlin, but that one has optional dependencies on Kotlin, which are basically met when quarkus-kotlin is included.

Couldn't we just add the optional dependencies on Kotlin to quarkus-scheduler? (I'm sorry if it's a stupid question ;-)

@geoand
Copy link
Contributor Author

geoand commented Apr 5, 2022

Couldn't we just add the optional dependencies on Kotlin to quarkus-scheduler? (I'm sorry if it's a stupid question ;-)

Not stupid at all!

We certainly could, but then that would end up creating problems for anyone wanting to work on quarkus-scheduler in an IDE that doesn't really support Kotlin (we had this problem in RESTEasy Reactive where Stef was using Eclipse and got to a point where he couldn't work on it until we moved the Kotlin stuff to a separate module)

@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2022

Couldn't we just add the optional dependencies on Kotlin to quarkus-scheduler? (I'm sorry if it's a stupid question ;-)

Not stupid at all!

We certainly could, but then that would end up creating problems for anyone wanting to work on quarkus-scheduler in an IDE that doesn't really support Kotlin (we had this problem in RESTEasy Reactive where Stef was using Eclipse and got to a point where he couldn't work on it until we moved the Kotlin stuff to a separate module)

Makes sense.

@geoand
Copy link
Contributor Author

geoand commented Apr 5, 2022

PR updated to fix a conflict

@geoand geoand requested a review from mkouba April 6, 2022 05:16
@geoand
Copy link
Contributor Author

geoand commented Apr 7, 2022

PR updated to fix a conflict

And now another one

mkouba
mkouba reviewed Apr 7, 2022
View reviewed changes
Copy link
Contributor

@mkouba mkouba 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 but it would be great if some kotlin expert could review the kotlin parts. CC @evanchooly

...s/scheduler/deployment/src/main/java/io/quarkus/scheduler/deployment/SchedulerProcessor.java Outdated Show resolved Hide resolved
...s/scheduler/deployment/src/main/java/io/quarkus/scheduler/deployment/SchedulerProcessor.java
@@ -553,4 +601,17 @@ UnremovableBeanBuildItem unremoveableSkipPredicates() {
return new UnremovableBeanBuildItem(new UnremovableBeanBuildItem.BeanTypeExclusion(SchedulerDotNames.SKIP_PREDICATE));
}

@BuildStep
void produceCoroutineScope(BuildProducer<AdditionalBeanBuildItem> buildItemBuildProducer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the kotlin extension should produce a io.quarkus.deployment.Capability that could be tested here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we should probably do that across Quarkus. But probably something for a follow up PR

...ns/scheduler/runtime/src/test/java/io/quarkus/scheduler/runtime/util/SchedulerUtilsTest.java Outdated Show resolved Hide resolved
mkouba
mkouba approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@mkouba mkouba 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.

@geoand geoand added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/spring Issues relating to the Spring integration area/dependencies Pull requests that update a dependency file labels Apr 8, 2022
@geoand geoand merged commit 610fb38 into quarkusio:main Apr 8, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 8, 2022
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Apr 8, 2022
@geoand geoand deleted the #21428 branch April 11, 2022 05:02
@geoand geoand mentioned this pull request Oct 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/scheduler kind/enhancement New feature or request
Projects
None yet
Milestone
2.9.0.CR1
Development

Successfully merging this pull request may close these issues.

Support kotlin coroutines in quarkus-scheduler
2 participants
@geoand @mkouba