Support multitenancy in id generators #3286
Conversation
gliljas
force-pushed
the
idgeneratormultitenancy
branch
from
6b58109
to
2f6d2ba
Compare
gliljas
changed the title
|
Not too keen on the exposing of properties in base classes and interfaces which are just for testing, such as "LastSourceValue" etc. Of course I kept them, but having them in the contract really doesn't serve any purpose. |
| @@ -145,27 +164,29 @@ public override object Generate(IAccessCallback callback) | |||
| { | |||
| using (_asyncLock.Lock()) | |||
There was a problem hiding this comment.
Ideally lock should be scoped to tenant too
There was a problem hiding this comment.
Done.
The way it's done now, the AsyncLock becomes part of the GenerationState, thereby piggybacking on that resolving part. It works fine, but the name GenerationState feels a bit off with that added. Maybe it's more a Context or something.
| public override long LastSourceValue | ||
| { | ||
| get { return _lastSourceValue; } | ||
| get { return _stateStore.NoTenantGenerationState.LastSourceValue; } |
There was a problem hiding this comment.
It should return -1 if not initialized. See
nhibernate-core/src/NHibernate/Id/Enhanced/IOptimizer.cs
Lines 24 to 26 in 79ad5b8
Your code throws. New method also doesn't return -1. It generates new state.
There was a problem hiding this comment.
Not sure I follow. Isn't it -1?
With "your code throws", are you referring to the throwing from NoTenantGenerationState? That was copied from Hibernate. The only option to avoid that is to generate new state.
| /// <summary> | ||
| /// Obtain the tenant identifier (multi-tenancy), if one, associated with this callback. | ||
| /// </summary> | ||
| string GetTenantIdentifier(); |
There was a problem hiding this comment.
I would say so, yes. Somehow the tenant identifer must become available to the optimizer. This is the way I cloned from https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/id/enhanced/AccessCallback.java , and I don't think there's a better way.
Fix for #2834
Looked quite a lot at the Java implementation, which has this support added. They have also cleaned up a bit, e.g by replacing old generators with configuration of new ones, but I didn't go that far.
Things to consider: