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

ArC and Vert.x: introduce InvokerFactoryBuildItem and use it in Vert.x event bus consumers #40815

Merged
merged 5 commits into from
Jun 10, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 23, 2024

The last commit fixes #26728

@Ladicek Ladicek requested review from cescoffier, mkouba and manovotn May 23, 2024 14:27
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/vertx labels May 23, 2024
@Ladicek
Copy link
Contributor Author

Ladicek commented May 23, 2024

Probably easiest to review each commit separately.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

We need to rework the dependency on Kotlin. Main extensions should not depend on it.

@@ -0,0 +1,133 @@
package io.quarkus.vertx.kotlin
Copy link
Member

Choose a reason for hiding this comment

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

Could it introduce an issue to depend on Kotlin here?

@@ -46,6 +46,10 @@
<groupId>io.quarkus</groupId>
<artifactId>quarkus-virtual-threads</artifactId>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-vertx-kotlin</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

That's not great. Now everything depends on Kotlin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a tiny artifact with 3 classes and transitive dependencies on artifacts that are already present, but I agree it is not nice. I took this over from the Scheduler extension without thinking too much about it. I'll ponder over it over the weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it's fine. It does not really bring the Kotlin sdk in (it's an optional dependency)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one more commit (to be squashed if you guys like it :-) ) that moves the dependency to the quarkus-kotlin module. Probably not very nice either, but should be better than a dependency on Kotlin in the quarkus-vertx module, I guess.

@mschorsch
Copy link
Contributor

Thank you @Ladicek for taking the time to improve the Kotlin support in Quarkus 👍

This comment has been minimized.

@Ladicek Ladicek force-pushed the arc-invoker-factory branch from 49f5072 to 9353a6a Compare May 29, 2024 10:02
@Ladicek Ladicek force-pushed the arc-invoker-factory branch from 9353a6a to ecd57ab Compare June 5, 2024 09:27
@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 5, 2024

Rebased and added one more commit (again, to be squashed) that turns the Kotlin support into an extension which is automatically present when both quarkus-kotlin and quarkus-vertx are present (due to https://quarkus.io/guides/conditional-extension-dependencies). Seems like the best way forward to me.

This comment has been minimized.

@Ladicek Ladicek force-pushed the arc-invoker-factory branch from ecd57ab to 7f78ce4 Compare June 5, 2024 09:45
@Ladicek Ladicek force-pushed the arc-invoker-factory branch from 7f78ce4 to 0900413 Compare June 5, 2024 09:57
@quarkus-bot quarkus-bot bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Jun 5, 2024

This comment has been minimized.

Copy link

github-actions bot commented Jun 5, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@Ladicek Ladicek force-pushed the arc-invoker-factory branch from 0900413 to 7b6b663 Compare June 6, 2024 07:19
@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 6, 2024

Rebased and squashed the last 3 commits.

@Ladicek Ladicek requested a review from mkouba June 6, 2024 07:24

This comment has been minimized.

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.

Went over ArC parts and those LGTM

This comment has been minimized.

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!

@Ladicek Ladicek force-pushed the arc-invoker-factory branch from 7b6b663 to ea2c6a4 Compare June 10, 2024 08:19
@Ladicek Ladicek added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 10, 2024
Copy link

quarkus-bot bot commented Jun 10, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit ea2c6a4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Jun 10, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit ea2c6a4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

@Ladicek Ladicek merged commit 8c35618 into quarkusio:main Jun 10, 2024
55 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 10, 2024
@Ladicek Ladicek deleted the arc-invoker-factory branch June 10, 2024 15:03
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 10, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jun 10, 2024
@geoand
Copy link
Contributor

geoand commented Jun 10, 2024

Very cool!

So I assume we should start using invokers wherever possible?

@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 11, 2024

That sounds reasonable, but at the same time, there are occasions where custom bytecode generation is more straightforward. I also vaguely recall that I couldn't use invokers in RESTEasy Reactive, for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/kotlin area/vertx kind/enhancement New feature or request triage/flaky-test
Projects
Development

Successfully merging this pull request may close these issues.

Support Kotlin suspend functions when using @ConsumeEvent
7 participants