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

Provides multi-tenancy capabilities for MongoDB with Panache #7431

Merged

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Feb 26, 2020

Fixes #5183

Documentation is missing, waiting for #6923 to be merged before updating the guide

@loicmathieu
Copy link
Contributor Author

@geoand with your changes on the MongoDB client build my multi-tenancy PR is now working 🎉

But I wonder if I can improve it:

  • I didn't consume any MongoDB related build items but the list of the needed MongoDB client can be known at build time by scanning the @MongoEntity annotations
  • I retrieve the MongoClient from Arc each time it's needed, I don't know if it is performant or not but maybe I can do this once for all inside a static intializer or inside a RUNTIME_INIT build step that store them via a recorder ... WDYT ?

@geoand
Copy link
Contributor

geoand commented Feb 26, 2020

@geoand with your changes on the MongoDB client build my multi-tenancy PR is now working

Glad to hear it!

But I wonder if I can improve it:

  • I didn't consume any MongoDB related build items but the list of the needed MongoDB client can be known at build time by scanning the @MongoEntity annotations

Essentially yes, this is why I don't like the MongoClienBuildItem and why I don't see the point of it. If you remember I had a PR that removed it at one point :)
The only consumer of MongoClientBuildItem seems to be Camel so they would have to vet for its removal.
I am of course +1 for removing

  • I retrieve the MongoClient from Arc each time it's needed, I don't know if it is performant or not but maybe I can do this once for all inside a static intializer or inside a RUNTIME_INIT build step that store them via a recorder ... WDYT ?

I don't think we need to go to those lengths. Retrieving from Arc should be just fine :)

@loicmathieu loicmathieu force-pushed the feat/mongo-panache-multi-tenancy branch 3 times, most recently from 9f1b04b to 88a7725 Compare February 26, 2020 14:58
@loicmathieu loicmathieu marked this pull request as ready for review February 26, 2020 14:58
@loicmathieu loicmathieu requested a review from FroMage March 4, 2020 09:59
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

The code LGTM, but I'm confused as to the relation between clients and databases.

IMO if you have two databases you get two clients: one for each DB, and they should not be addressed by client name but by DB name. But perhaps this is different in Mongo as it is in ORM?

@loicmathieu
Copy link
Contributor Author

@FroMage In MongoDB you can have multiple databases by MongoDB instance, all would be accessible using the same client (it's closest to the schema of a JDBC database than to a database).
Multiple clients is really multiple database hosts (clusters in fact).

For me, with JDBC, you create a connection by database, not to a database engine, so it's different. You can have multiple database in a single Postgres server but you will need to create separate connections for them ...

That's why we choose the name client and not connection to clearly make the distinction between the two.

Maybe the documentation can be improved a bit,

@FroMage
Copy link
Member

FroMage commented Mar 6, 2020

OK, in that case, IF it's true that mongo users are fine understanding that a mongo DB is an SQL schema, and a mongo client is a JDBC connection, then sure, keep that terminology. Not sure it's worth documenting it for SQL/JDBC people like me, if it's such common knowledge for mongo users. After all, this extension assumes you have some knowledge of mongo :)

@loicmathieu
Copy link
Contributor Author

Not sure it's worth documenting it for SQL/JDBC people like me, if it's such common knowledge for mongo users

Maybe we can ask @geoand if the current wordings are clear enought, more eyes, better naming ;)

After all, this extension assumes you have some knowledge of mongo :)

Yes, but as we aim to provides very easy to use API, just a little knowledge should be enought ;)

@geoand
Copy link
Contributor

geoand commented Mar 6, 2020

@FroMage In MongoDB you can have multiple databases by MongoDB instance, all would be accessible using the same client (it's closest to the schema of a JDBC database than to a database).
Multiple clients is really multiple database hosts (clusters in fact).

For me, with JDBC, you create a connection by database, not to a database engine, so it's different. You can have multiple database in a single Postgres server but you will need to create separate connections for them ...

That's why we choose the name client and not connection to clearly make the distinction between the two.

I think this is great information to have in the documentation. I don't think this is exactly common knowledge for most people.
Your comparison to JDBC is great is because that is what everyone knows - I would to have that knowledge preserved in the guide.

Perhaps a follow up PR?

@loicmathieu
Copy link
Contributor Author

Perhaps a follow up PR?

You now it will imply as much work for you to correct it than for me to write it ;)

I'll add it to my TODO list ...

@geoand
Copy link
Contributor

geoand commented Mar 6, 2020

Perhaps a follow up PR?

You now it will imply as much work for you to correct it than for me to write it ;)

And then you spotting the missing words in my corrections 😆

@loicmathieu
Copy link
Contributor Author

And then you spotting the missing words in my corrections laughing

Team works !

@loicmathieu loicmathieu added this to the 1.4.0 milestone Mar 10, 2020
@loicmathieu
Copy link
Contributor Author

@geoand this PR is validated. Can you merge it ?

@geoand
Copy link
Contributor

geoand commented Mar 19, 2020

Sure, I forgot about it!

Thanks a lot!

@geoand geoand merged commit f901588 into quarkusio:master Mar 19, 2020
@loicmathieu loicmathieu deleted the feat/mongo-panache-multi-tenancy branch March 19, 2020 13:22
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.

Multitenancy with mongodb panache extension
3 participants