Hibernate ORM Multitenancy

[michael-schnell]

Fixes #5681

[michael-schnell]

@geoand: One of the checks failed, but it seems to be a build server problem

[geoand]

Yeah, one of the steps timed out, it has nothing to do with this PR.

[michael-schnell]

Yeah, one of the steps timed out, it has nothing to do with this PR.

Will someone restart that step or how is the procedure?

[geoand]

I restarted it

[michael-schnell]

@machi1990 Can you invite the reviewers again? (First pull request had problems and I started this new one)

[FroMage]

I'm not sure I'm qualified to review this PR. But I thought Hibernate could not use multiple entity managers for multiple data sources yet? This seems to be based around the fact that it can. Did we already add support for that, @Sanne ?

[michael-schnell]

I'm not sure I'm qualified to review this PR. But I thought Hibernate could not use multiple entity managers for multiple data sources yet? This seems to be based around the fact that it can. Did we already add support for that, @Sanne ?

Multiple data sources is already supported by Quarkus:
https://quarkus.io/guides/datasource#multiple-datasources

[FroMage]

Ah, it's multiple persistence units that it doesn't support: #2835

[machi1990]

Ah, it's multiple persistence units that it doesn't support: #2835

@FroMage That's why (#2835) I asked a review from you too :-)

[michael-schnell]

Ah, it's multiple persistence units that it doesn't support: #2835

Yes, but when it is finally supported, it would be easily possible to enhance the multitenancy to use persistence units in addition to multiple data sources.

[michael-schnell]

Any chance that we get this PR into 1.4.0?
I would like to use it in my code, but without a release this is difficult.

[geoand]

I'm not the one that decides, but I think that would be difficult. We already have a CR for 1.4 and we usually only tend to add fixes into the Final.

But like I said, it's not up to me 😎

[Sanne]

sorry for the delay @michael-schnell , finally getting to try this out

[michael-schnell]

finally getting to try this out

@Sanne How is it going?

[michael-schnell]

Any chance to get this some day done? It is now two weeks since the pull request was made and there is no real feedback til now. I know the situation is special, but this is not a thousands-of-lines codes review. It's just a few files, easy to test and should be a matter of half an hour or hour. Sorry folks, but this is really annoying.

[Sanne]

bummer, sorry I had some early feedback but it seems I didn't publish it. Back to it now..

[Sanne]

I didn't get to review the implementation details - however that's less important, it can always be refined in a second time.

I have some questions about the API we want to expose though. Added them in the review.

[michael-schnell]

@gsmet @FroMage @emmanuelbernard @Sanne Can someone please continue with the review - I started working on this more than a month ago. The pull request is now also already three weeks old. I changed several things on demand of @Sanne and now again no response since 7 days. Looks like you are not really interested in people doing some work. Please let's continue to close this never ending story!

[FroMage]

We are always very interested in contributions. But we're also all very busy on a lot of other things, not to mention disrupted by the current pandemic. So please be patient. It's not unusual for PRs to take a few weeks or months. What matters is that at the end they get merged and are much better than before reviews took place.
Don't lose hope :)

[emmanuelbernard]

We are always very interested in contributions. But we're also all very busy on a lot of other things, not to mention disrupted by the current pandemic. So please be patient. It's not unusual for PRs to take a few weeks or months. What matters is that at the end they get merged and are much better than before reviews took place.
Don't lose hope :)

Right, the specific difficulty here is that you are touching on an area with lots of implications and ramifications. So a casual check / optimistic push is out of the equation. This low level detail is only know intimately enough by few people. That's why we prefer to wait for Sanne's feedback to get a good solution out.

[michael-schnell]

@Sanne 14 more days passed since the last commit - Any chance to do the review?

[Sanne]

hi @michael-schnell , sure I'm back on this this afternoon. checking it out..

[Sanne]

Great progress! I think it's almost ready, so started to look into more tiny details of the implementation - hence more fine grained feedback this time.

The bigger elefant in the room is the lack of any integration tests. Do you think you could add at least one?
Just covering one of the use cases from your documentation would give reasonable confidence IMO.

And thanks for the awesome docs!

Thanks

[michael-schnell]

Great progress! I think it's almost ready, so started to look into more tiny details of the implementation - hence more fine grained feedback this time.

The bigger elefant in the room is the lack of any integration tests. Do you think you could add at least one?
Just covering one of the use cases from your documentation would give reasonable confidence IMO.

I will work on it over the weekend.

[michael-schnell]

@Sanne All changes are now commited, including the new 'hibernate-tenancy' integration test.
I used the 'jpa-mariadb' as template for the test. It states "By default, the tests of this module are disabled." - I hope this is also OK for the new 'hibernate-tenancy' integration test.

[Sanne]

Hi @michael-schnell , regarding a question you had regarding the "The @PathParam is somehow not passed correctly." you mentioned in a comment (which I can't find on the github UI now) - did you solve it?

In case, it might relate to the compiler settings; you need to have the parameters=true configuration property on the compiler plugin, like we have it in most quickstarts:

        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <version>${compiler-plugin.version}</version>
            <configuration>
                <source>1.8</source>
                <target>1.8</target>
                <!-- the parameters=true option is critical so that RESTEasy works fine -->
                <parameters>true</parameters>
            </configuration>
        </plugin>

Awesome progress on the other commits, I'll check it out in more detail tonight.

[michael-schnell]

Hi @michael-schnell , regarding a question you had regarding the "The @PathParam is somehow not passed correctly." you mentioned in a comment (which I can't find on the github UI now) - did you solve it?

Tnx, I solved it 5 minutes after I posted the comment (That's why I deleted it). It was a strange behaviour that was different between the integration test project and the quickstart example. I forgot to add the name of the parameter in the "@PathParam" annotation. So it was something like @PathParam String myVar instead of @PathParam("myVar") String myVar. It worked for the quickstart project (that's were I copied it from), but failed in the integration test. I was searching for hours for what was wrong... Fortunately finally I got it.

[Sanne]

Haven't fully finished the review, but since it looks great so far:

could you amend the expected MariaDB configuration? The used database password is not consistent with other modules, it would be great if you could use exactly the same configuration so that the CI infrastructure can provide a single DB instance for all testing needs.

See

quarkus/.github/workflows/ci-actions.yml

Lines 141 to 149 in df66ad4

	mariadb:
	image: mariadb:10.4
	env:
	MYSQL_USER: hibernate_orm_test
	MYSQL_PASSWORD: hibernate_orm_test
	MYSQL_DATABASE: hibernate_orm_test
	MYSQL_RANDOM_ROOT_PASSWORD: true
	ports:
	- 127.0.0.1:3308:3306

[michael-schnell]

@geoand Can you restart the build job? Seems as if something timed out that is not related to the changes.

[geoand]

Done

[michael-schnell]

could you amend the expected MariaDB configuration? The used database password is not consistent with other modules, it would be great if you could use exactly the same configuration so that the CI infrastructure can provide a single DB instance for all testing needs.

Which one do you mean exactly? The test needs to create two new databases for the tenants in the MariaDB instance. So I need a user that is allowed to do that. Only the root user normally has the necessary rights. That's why the default datasource has a different user than the two other tenant data sources.

Must be root to be able to create new DBs:

# Default DB Configuration
quarkus.datasource.db-kind=mariadb
quarkus.datasource.username=root
quarkus.datasource.password=secret

The other two are created by src/main/resources/database/default/V1.0.0__init_databases.sql:

# DATABASE Tenant 'base' Configuration
quarkus.datasource.base.db-kind=mariadb
quarkus.datasource.base.username=jane
quarkus.datasource.base.password=abc

# DATABASE Tenant 'mycompany' Configuration
quarkus.datasource.mycompany.db-kind=mariadb
quarkus.datasource.mycompany.username=john
quarkus.datasource.mycompany.password=def

Unfortunately only one database (the "hibernate_orm_test") will get created automatically. The other ones are therefore created by the script including the users and rights.

[michael-schnell]

Done

Tnx. Unfortunately now the another one fails that was fine before... :-(

[michael-schnell]

@geoand Another try? Seems to be unstable...

[Sanne]

Which one do you mean exactly? The test needs to create two new databases for the tenants in the MariaDB instance. So I need a user that is allowed to do that. Only the root user normally has the necessary rights. That's why the default datasource has a different user than the two other tenant data sources.

ah right, thanks! I hadn't seen the integration test yet - now I understand and agree.

So we'll need to adapt CI for this - but it's ok we can follow up on that as a separate issue if you prefer. I've opened #9389 - we recently changed CI system so I'm not overly familiar with it yet so I might try volunteering for that :)

[Sanne]

Merged! Many thanks @michael-schnell !