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

Support multitenancy in id generators #3286

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gliljas
Copy link
Member

@gliljas gliljas commented Apr 10, 2023

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:

  • The IAccessCallback interface was modified, i.e. a breaking change. Could be handled with an intermediate extension interface.
  • The DoWorkInNewTransaction/TransactionHelper inheritance is super weird. Why not just use a delegate?
  • All the optimizers are nested inside OptimizerFactory.cs. This serves no purpose at all, and I would very much like to split that file. Right now, that would make code review harder, though.

@gliljas gliljas changed the title WIP: Support multitenancy in id generators Support multitenancy in id generators Apr 11, 2023
@gliljas
Copy link
Member Author

gliljas commented Apr 11, 2023

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())
Copy link
Member

Choose a reason for hiding this comment

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

Ideally lock should be scoped to tenant too

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

src/NHibernate/Id/TenantStateStore.cs Outdated Show resolved Hide resolved
public override long LastSourceValue
{
get { return _lastSourceValue; }
get { return _stateStore.NoTenantGenerationState.LastSourceValue; }
Copy link
Member

Choose a reason for hiding this comment

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

It should return -1 if not initialized. See

/// The last value we obtained from the underlying source; -1 indicates we have not yet consulted with the source.
/// </value>
long LastSourceValue { get; }

Your code throws. New method also doesn't return -1. It generates new state.

Copy link
Member Author

Choose a reason for hiding this comment

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

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();
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@hazzik hazzik self-requested a review June 6, 2024 12:13
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.

3 participants