-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Enable Hibernate ORM multitenancy IT on CI #9390
Conversation
Pushed again, as Flyway needs to take into account that other integration tests might have used the DB before this module was run. |
Could you put a slightly higher timeout for Data3? It's cancelled and considering the changes you made, I'm not comfortable merging this without it being green. Thanks. |
I thought this was running in Data1 ?
…On Mon, 18 May 2020, 16:05 Guillaume Smet, ***@***.***> wrote:
Could you put a slightly higher timeout for Data3? It's cancelled and
considering the changes you made, I'm not comfortable merging this without
it being green.
Thanks.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKMTPDFXX46S4YPNLDDILRSFFB3ANCNFSM4NDRMW5Q>
.
|
LOG.debugv("selectConnectionProvider({0})", tenantIdentifier); | ||
|
||
ConnectionProvider provider = providerMap.get(tenantIdentifier); | ||
if (provider == null) { | ||
return providerMap.computeIfAbsent(tenantIdentifier, tid -> resolveConnectionProvider(tid)); | ||
final ConnectionProvider connectionProvider = resolveConnectionProvider(tenantIdentifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to do this? It could introduce a race condition, because multiple parallel requests will use the same object. I think this is not thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fine to me - this is indeed a race condition but it's benign, as the end result doesn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct. I just thought the "computeIfAbsent" is a more accurate usage of the API and does not cause such questions, because it is defined by the API of the hash map implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you - your solution was more elegant. But we try to avoid lambdas for efficiency reasons. Of course we're not extreme about it: lambdas are a fine solution when they are a good solution, but I still tend to prefer avoiding them in such situations.
Rebased. Let's wait for CI. |
Fixes #9389
The multi-tenancy integration tests require root access to the MariaDB instance, so that it can create multiple databases to test the dybanamic DB switching ability.
The integration tests of these modules were not enabled so far.