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

Multitenancy support #754

Closed
yaakov-berkovitch opened this issue May 13, 2021 · 6 comments · Fixed by #757 or #761
Closed

Multitenancy support #754

yaakov-berkovitch opened this issue May 13, 2021 · 6 comments · Fixed by #757 or #761
Assignees
Milestone

Comments

@yaakov-berkovitch
Copy link

Hi,

according to the documentation, to add support for multitenancy, the following must be done:

  1. define your own implementation of ReactiveConnectionPool or extend DefaultSqlClientPool and override getTenantPool(String tenantId).
  2. a class which implements CurrentTenantIdentifierResolver
  3. multitenancy strategy: database or schema

So, regarding (1), as you recommend, I tried to extend the DefaultSqlClientPool and create a pool for each tenant.
It seems that I have a big misunderstanding. I need a pool per URI, and keep them into a map with the tenant-id as the key, so the getTenantPool will return the correct pool for the tenant.
But I cannot use the createPools(URI) to create the pool per tenant because the ThreadLocalPoolManager is not public, and so I have no access to this class.

I would very appreciate some lights how to do it.

Thanks.

@gavinking
Copy link
Member

Errrm ... @Sanne?

This issue might have slipped through the cracks, since I worked on multitenancy, and then much later Sanne did the ThreadLocalPoolManager.

@Sanne
Copy link
Member

Sanne commented May 13, 2021

I coincidentally just started looking at vertx 4.1 and its new pool, with the plan of removing our ThreadLocalPoolManager so I guess that will help.

To prevent such regressions we need to add a test covering Multitenancy though.

@gavinking
Copy link
Member

Well, there is a test (ReactiveMultitenantTest), but I guess it just doesn't touch whatever is causing this problem.

@DavideD
Copy link
Member

DavideD commented May 14, 2021

I will have a look

@DavideD DavideD self-assigned this May 14, 2021
@yaakov-berkovitch
Copy link
Author

I also looked at this test, but it is not testing multitenancy . The custom pool (as shown below) you implemented in your test is not creating different pool per tenant:

public class MySqlClientPool extends DefaultSqlClientPool {
    @Override
    protected Pool getTenantPool(String tenantId) {
        return getPool();
    }
}

@DavideD
Copy link
Member

DavideD commented May 14, 2021

Yeah... It's a very simple test

DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 14, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 14, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 14, 2021
We only use it to convert a `Future` to a `CompletionStage` but
there is already a `Future#toCompletionStage`.
No reason to keep it around.
DavideD added a commit that referenced this issue May 17, 2021
We only use it to convert a `Future` to a `CompletionStage` but
there is already a `Future#toCompletionStage`.
No reason to keep it around.
@DavideD DavideD added this to the 1.0.0.CR5 milestone May 17, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 18, 2021
The test for multitenancy needs to be able to create
additional databaseses but it needs the right permissions
if the user starts Postgres without Docker/Podman.
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 18, 2021
This way we can use them with `@AfterClass` and `@BeforeClass`
if we need it for a test
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 18, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue May 18, 2021
@DavideD DavideD linked a pull request May 18, 2021 that will close this issue
gavinking pushed a commit that referenced this issue May 18, 2021
The test for multitenancy needs to be able to create
additional databaseses but it needs the right permissions
if the user starts Postgres without Docker/Podman.
gavinking pushed a commit that referenced this issue May 18, 2021
This way we can use them with `@AfterClass` and `@BeforeClass`
if we need it for a test
gavinking pushed a commit that referenced this issue May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants