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

Use the MongoDB Java driver's DNS SPI to enable mongo+srv:// connection strings in native mode #26388

Merged
merged 1 commit into from
Jul 27, 2022
Merged

Use the MongoDB Java driver's DNS SPI to enable mongo+srv:// connection strings in native mode #26388

merged 1 commit into from
Jul 27, 2022

Conversation

evanchooly
Copy link
Member

fixes #26387

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/mongodb labels Jun 27, 2022
@evanchooly evanchooly marked this pull request as ready for review June 27, 2022 20:11
loicmathieu
loicmathieu requested changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

I'm not an expert on DNS so if @cescoffier can have a look at the DNS part it would be great.

I added some suggestion on code simplification but overall it looks good to me.

I wonder if the config related code could be simplified by moving all the default options as default inside MongodbConfig and recovering the MongodbConfig bean from Arc. This would allow to directly use the config attribute instead of using the Config object.

integration-tests/mongodb-client/pom.xml Outdated Show resolved Hide resolved
integration-tests/mongodb-client/src/main/java/io/quarkus/it/mongodb/ReactiveBookResource.java Outdated Show resolved Hide resolved
...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/dns/MongoDnsClient.java Outdated Show resolved Hide resolved
...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/dns/MongoDnsClient.java Outdated Show resolved Hide resolved
...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/dns/MongoDnsClient.java Outdated Show resolved Hide resolved
...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/dns/MongoDnsClient.java Outdated Show resolved Hide resolved
@loicmathieu loicmathieu self-requested a review June 29, 2022 07:46
loicmathieu
loicmathieu approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

@evanchooly as the code is a copy from the substitution, I approve the PR so if you don't have time to look at my other comments feel free to merge it.

cescoffier
cescoffier previously requested changes Jun 29, 2022
View reviewed changes
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.

If I understand correctly, this PR switches to the Vert.x resolver in JVM and native mode.
I think we should provide an option where we allow using the default (JNDI-based) one in JVM mode, at least for compatibility purpose.

...godb-client/deployment/src/main/java/io/quarkus/mongodb/deployment/MongoClientProcessor.java Outdated Show resolved Hide resolved
extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongodbConfig.java Outdated Show resolved Hide resolved
...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/dns/MongoDnsClient.java Outdated Show resolved Hide resolved
extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongodbConfig.java Show resolved Hide resolved
@evanchooly
Copy link
Member Author

If I understand correctly, this PR switches to the Vert.x resolver in JVM and native mode. I think we should provide an option where we allow using the default (JNDI-based) one in JVM mode, at least for compatibility purpose.

I was wondering about this but since it's SPI driven that gets rather complicated. I suppose we could switch inside the DnsProvider and that's reasonably easy to do. Personally, I kind of like the consistency of using it exclusively.

@evanchooly
Copy link
Member Author

If I understand correctly, this PR switches to the Vert.x resolver in JVM and native mode. I think we should provide an option where we allow using the default (JNDI-based) one in JVM mode, at least for compatibility purpose.

I was wondering about this but since it's SPI driven that gets rather complicated. I suppose we could switch inside the DnsProvider and that's reasonably easy to do. Personally, I kind of like the consistency of using it exclusively.

What if we do a class availability check on the JNDI types, use the driver's stuff if JNDI is present, or this "new" stuff if it's not? Then in JVM mode, it'd just use the driver's stuff and everything's peachy. but in native mode, JNDI wouldn't be there so we'd use this?

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@evanchooly evanchooly marked this pull request as draft July 25, 2022 18:00
@quarkus-bot

This comment has been minimized.

Sign in to view
@evanchooly evanchooly marked this pull request as ready for review July 25, 2022 19:38
@evanchooly
Copy link
Member Author

i took a detour on other things but this is ready, i think.

@evanchooly
Copy link
Member Author

@cescoffier what do you think of this now?

@evanchooly evanchooly merged commit 8e41c5c into quarkusio:main Jul 27, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Jul 27, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 27, 2022
@evanchooly evanchooly deleted the mongoDns branch July 27, 2022 21:52
@geoand geoand mentioned this pull request Sep 1, 2022
Closed
@geoand geoand mentioned this pull request Oct 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radcortez radcortez radcortez left review comments

@loicmathieu loicmathieu loicmathieu approved these changes

@cescoffier cescoffier Awaiting requested review from cescoffier

Assignees
No one assigned
Labels
area/dependencies Pull requests that update a dependency file area/mongodb kind/enhancement New feature or request
Projects
None yet
Milestone
2.12.0.CR1
Development

Successfully merging this pull request may close these issues.

Use the MongoDB Java driver's DNS SPI to enable mongo+srv:// connection strings in native mode
4 participants
@evanchooly @cescoffier @loicmathieu @radcortez