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

Don't create default mongo clients when not needed and fix @MongoClientName qualifier #11568

Merged
merged 2 commits into from
Aug 27, 2020
Merged

Don't create default mongo clients when not needed and fix @MongoClientName qualifier #11568

merged 2 commits into from
Aug 27, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 24, 2020 •
edited
Loading

Fixes: #11491

@geoand geoand requested a review from loicmathieu August 24, 2020 13:41
@geoand geoand mentioned this pull request Aug 24, 2020
Closed
@loicmathieu
Copy link
Contributor

Is iterating over all the injection point has a cost ? I don't know how many of injection points exists in a classical application so I dont't know if it can add a lot of build time cost.

This will not work for MongoDB with Panache as we set all clients unremovable.
Maybe it's not something that we should fix but instead providing a workaround by setting the default client hostsproperty to empty to disable the default MongoDB client:

# disable the default MongoDB client
quarkus.mongodb.hosts=

@geoand
Copy link
Contributor Author

geoand commented Aug 24, 2020

Is iterating over all the injection point has a cost ? I don't know how many of injection points exists in a classical application so I dont't know if it can add a lot of build time cost.

The cost is negligible. This same operation is performed in other extensions as well.

This will not work for MongoDB with Panache as we set all clients unremovable.
Maybe it's not something that we should fix but instead providing a workaround by setting the default client hostsproperty to empty to disable the default MongoDB client:

# disable the default MongoDB client
quarkus.mongodb.hosts=

Yeah, as you said all will be made unremovable, but that should not be a problem.

@loicmathieu
Copy link
Contributor

OK, so it fixes the issue only for direct usage of the MongoDB client not via Panache.

loicmathieu
loicmathieu approved these changes Aug 24, 2020
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.

LGTM

@geoand
Copy link
Contributor Author

geoand commented Aug 24, 2020

OK, so it fixes the issue only for direct usage of the MongoDB client not via Panache.

It only fixes a part unfortunately. There is still the CDI related part that I mentioned in the issue that we need Martin to look at (I could of course, but it would take me a lot longer than Martin :))

@geoand geoand changed the title Don't create default mongo clients when not needed Don't create default mongo clients when not needed and fix MongoClientName qualifier Aug 24, 2020
@geoand geoand changed the title Don't create default mongo clients when not needed and fix MongoClientName qualifier Don't create default mongo clients when not needed and fix @MongoClientName qualifier Aug 24, 2020
mkouba
mkouba approved these changes Aug 24, 2020
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.

gsmet
gsmet requested changes Aug 25, 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.

Added some suggestion.

Also what worries me here is that we aren't consistent with the behavior elsewhere.

I think it all boils down to the "we need to be able to know if a runtime property will be set" discussion.

...lient/deployment/src/main/java/io/quarkus/mongodb/deployment/MongoClientBuildTimeConfig.java Outdated Show resolved Hide resolved
@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2020

Added some suggestion.

Also what worries me here is that we aren't consistent with the behavior elsewhere.

I think it all boils down to the "we need to be able to know if a runtime property will be set" discussion.

Not really... If you know the injection points (which we do), then you know which mongo clients you need. Unless I am missing something?

@geoand geoand requested a review from gsmet August 25, 2020 08:43
@gsmet
Copy link
Member

gsmet commented Aug 25, 2020

What if you programmatically get the bean? Isn't that a possibility?

@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2020

This works for Panache and Camel because in those cases the bean does get created.

For users that would want to do that, then the new setting can be used.

@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2020

@gsmet is this OK with you?

@geoand geoand added this to the 1.8.0 - master milestone Aug 25, 2020
gsmet
gsmet requested changes Aug 27, 2020
View reviewed changes
...godb-client/deployment/src/main/java/io/quarkus/mongodb/deployment/MongoClientProcessor.java
BeanDefiningAnnotationBuildItem registerConnectionBean() {
return new BeanDefiningAnnotationBuildItem(MONGOCLIENT_ANNOTATION);
}
private static final DotName MONGO_CLIENT_ANNOTATION = DotName.createSimple(MongoClientName.class.getName());
Copy link
Member

Choose a reason for hiding this comment

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

If it didn't work before, please let's move this annotation to the API package i.e. no runtime.. It's user API.

Also we will probably need to update the doc and quickstart if it's mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed to do this in a follow up

@gsmet gsmet merged commit b87ea49 into quarkusio:master Aug 27, 2020
@geoand geoand deleted the #11491 branch August 27, 2020 11:31
geoand added a commit to geoand/quarkus that referenced this pull request Aug 27, 2020
@geoand
Use proper package for @MongoClientName
10bbaed 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
@geoand geoand mentioned this pull request Aug 27, 2020
Merged
geoand added a commit to geoand/quarkus that referenced this pull request Aug 27, 2020
@geoand
Use proper package for @MongoClientName
45af8ef 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
geoand added a commit to geoand/quarkus that referenced this pull request Aug 27, 2020
@geoand
Use proper package for @MongoClientName
a450bda 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
geoand added a commit to geoand/quarkus that referenced this pull request Aug 27, 2020
@geoand
Use proper package for @MongoClientName
186de5d 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
gsmet pushed a commit to geoand/quarkus that referenced this pull request Aug 27, 2020
@geoand @gsmet
Use proper package for @MongoClientName
f5a521e 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
geoand added a commit to geoand/quarkus that referenced this pull request Aug 28, 2020
@geoand
Use proper package for @MongoClientName
1873753 
For now we just deprecate the one annotation and support both

Relates to: quarkusio#11568 (comment)
@gsmet
Copy link
Member

gsmet commented Sep 7, 2020

This one has been backported via a specific PR due to conflicts: #11849 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loicmathieu loicmathieu loicmathieu approved these changes

@gsmet gsmet gsmet approved these changes

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/mongodb
Projects
None yet
Milestone
1.8.0.CR1
Development

Successfully merging this pull request may close these issues.

Named mongo client injection trying to use default mongo client
4 participants
@geoand @loicmathieu @gsmet @mkouba