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

[Enhancement] Tries to inject Vertx instance from CDI using a qualifier #2772

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gaol
Copy link

@gaol gaol commented Sep 27, 2024

Fixes #2771

This PR tries to add 2 config items:

  • mp.messaging.connector.vertx.cdi.qualifier, which is used to tries to inject the Vertx instance with the specified qualifier if configured.
  • mp.messaging.connector.vertx.cdi.qualifier.required, boolean, which is used to indicate if the qualifier is required in case it comes from CDI, otherwise it will create a new Vertx instance like what current does.

ExecutionHolder is used by both WildFly and Quarkus. And for WildFly, there is a coming vertx subsystem to be able to configure the VertxOptions following the WildFly management model style, which will expose a CDI bean with a qualifier so that other subsystems can select it by themselves. The 2nd config item can be included in the integration code of WildFly so that applications deployed to WildFly will inject only the instance with a qualifier, and leave the use case in Quarkus not changed at all.

@@ -19,6 +22,9 @@
@ApplicationScoped
public class ExecutionHolder {

private static final String REACTIVE_MESSAGING_VERTX_CDI_QUALIFIER = "mp.messaging.connector.vertx.cdi.qualifier";
private static final String REACTIVE_MESSAGING_VERTX_CDI_QUALIFIER_REQUIRED = "mp.messaging.connector.vertx.cdi.qualifier.required";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need a second property ?
Can't we just look for "mp.messaging.connector.vertx.identifier" and if it is defined look up the instance with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, just the identifier value should be fine.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe not using Identifier, I will test to confirm that. The case it tries to cover is when there is a vertx instance exposed with a cdi qualifier(Named("vertx")) but the Instance<Vertx> ist in the ExecutionHolder, the ist.isUnsatisfied() is false, which forces sr rm to inject the instance.

BTW, I will be on holidays this week, will report back next week, thanks :)

Copy link
Author

Choose a reason for hiding this comment

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

Done by using only the first option and the @io.smallrye.common.annotation.Identifier qualifier only.

The vertx bean from the vertx subsystem is exposed with 2 qualifiers: @Any and @Identifier, so to select the instance only when the cdi qualifier is specified, I split the @Default out in the else check block.

@gaol
Copy link
Author

gaol commented Oct 9, 2024

Thank you all for the reviews, I updated the PR, would you please review again ? thank you :)

@gaol
Copy link
Author

gaol commented Oct 14, 2024

@cescoffier @ozangunalp would you please review again ? thanks :)

Copy link
Collaborator

@ozangunalp ozangunalp left a comment

Choose a reason for hiding this comment

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

A couple of tests in the provider module would be great!

@gaol
Copy link
Author

gaol commented Oct 16, 2024

@ozangunalp thank you for the reviews, I updated to add a test in the provider module, please let me know if there is anything needs to be fixed or improved. :)

@gaol gaol requested a review from ozangunalp October 16, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Tries to inject Vertx instance from CDI using a qualifier
4 participants