Elfocrash / Cosmonaut

🌐 A supercharged Azure CosmosDB .NET SDK with ORM support
https://cosmonaut.readthedocs.io
MIT License
342 stars 44 forks source link

CosmosStore should be singleton, but has fixed collection name once created #71

Closed Mortana89 closed 5 years ago

Mortana89 commented 5 years ago

Hello,

We're trying to use Cosmonaut in our multi-tenant application. Each tenant has it's own collections, and such we need to be able to, during runtime, specify the collection name. Not only during general HTTP requests, but also during background work (events, messages, ...). I read in the documentation that CosmosStore should preferably be registered as a singleton per entity type. In our case that's not possible because the collection name needs to be provided when the cosmosstore is created.

What would be the preferred way going forward? One approach I was thinking, was registering a singleton factory that caches cosmosstores for tenants, and creates the correct cosmosstore once requested...

Elfocrash commented 5 years ago

Hello @Mortana89. The CosmosStore should still be a singleton in this scenario. You should instead make a facade over it to add you multi tenant business logic with appropriate predicates to limit the requests to a tenant per tenant scope.

Does this make sense?

(Sorry but I am currently on holiday so replies might be slow)

Mortana89 commented 5 years ago

Thanks for the fast reply, even on a holiday! ;)

The CosmosStore itsself can still act as a singleton, with a factory on top of it yes. I'm writing something like this:

public class CosmosStoreFactory : ICosmosStoreFactory
    {
        private static ConcurrentDictionary<string, ConcurrentDictionary<string, object>> _cosmosStores =
        new ConcurrentDictionary<string, ConcurrentDictionary<string, object>>();
        private readonly MeterDataCosmosSettings _cosmosSettings;

        public CosmosStoreFactory(MeterDataCosmosSettings cosmosSettings)
        {
            _cosmosSettings = cosmosSettings ?? throw new ArgumentNullException(nameof(cosmosSettings));
        }
        public ICosmosStore<TEntity> GetCosmosStore<TEntity>(string tenantId)
            where TEntity : class
        {
            if(!_cosmosStores.ContainsKey(tenantId))
            {
                _cosmosStores.TryAdd(tenantId, new ConcurrentDictionary<string, object>());
            }

            if (!_cosmosStores[tenantId].ContainsKey(typeof(TEntity).Name))
            {
                var newStore = new CosmosStore<TEntity>(_cosmosSettings, tenantId);
                _cosmosStores[tenantId].TryAdd(typeof(TEntity).Name, newStore);

                return newStore;
            }
            else
            {
                return (ICosmosStore<TEntity>)_cosmosStores[tenantId][typeof(TEntity).Name];
            }
        }
    }

Will need to test this though. I'm not worried about limiting the requests per tenant scope though, as we're using Mediatr as a pipeline, and we're injecting a preprocessor there that is injecting tenant IDs in the correct place.

Elfocrash commented 5 years ago

I see nothing wrong with this. It will be both fast and reliable. The only thing is that the CosmosStores need to be initialised when they are added in there to prevent the initial latency of the initialisation.

Other than that time implementation looks sound.

Elfocrash commented 5 years ago

@Mortana89 am I ok to close this one?

Mortana89 commented 5 years ago

Yes sure, was more a discussion. Thanks for your support!

brettveenstra commented 4 years ago

actually ran into a Socket exhaustion issue using Cosmonaut in an Azure function... Microsoft also recommends re-using DocumentClient instances across an App Domain - see https://docs.microsoft.com/en-us/azure/architecture/antipatterns/improper-instantiation/

... was building a ConcurrentDictionary cache when found this thread 😃

@Elfocrash: maybe you might want to provide some guidance and/or structures around this?

Thanks for a nice library overall!

Elfocrash commented 4 years ago

Hey @brettveenstra

This was one of my toughest design decisions when i originally wrote Cosmonaut and as with many things when you are designing a system, I regret taking it.

It would be easier for me to just reuse the same DocumentClient than creating a new one per CosmosStore but I didn't wanna limit the user to a single configuration since you might wanna have different settings on a per CosmosStore scenario. This however ended up begin the edge case and not the norm. I can't pull back on this decision since there are many people with apps in prod using Cosmonaut.

What I think I'll do is to just hash the settings and automatically reuse a single DocumentClient for CosmosStores with the exact same infrastructure settings. In the meantime I might just add Mortana's solution in the Docs.

Mortana89 commented 4 years ago

In case you're interested, I've polished the factory a little bit to also support multiple 'datasources' aka databases;

`public class CosmosStoreFactory : ICosmosStoreFactory { private static ConcurrentDictionary<string,ConcurrentDictionary<string, ConcurrentDictionary<string, object>>> _cosmosStores = new ConcurrentDictionary<string,ConcurrentDictionary<string, ConcurrentDictionary<string, object>>>();

    private List<CosmosStoreSettings> _dataSources;

    public CosmosStoreFactory(List<CosmosStoreSettings> dataSources)
    {
        _dataSources = dataSources ?? throw new ArgumentNullException(nameof(dataSources));
    }

    public CosmosStore<TEntity> GetCosmosStore<TDataSource,TEntity>(string tenantId)
        where TEntity : AggregateEntity
        where TDataSource : CosmosStoreSettings
    {
        var dataSource = _dataSources.SingleOrDefault(x => x.GetType() == typeof(TDataSource));

        if (dataSource == null)
            throw new ArgumentException("Could not find the questioned datasource");

        _cosmosStores.GetOrAdd(typeof(TDataSource).Name, x => new ConcurrentDictionary<string, ConcurrentDictionary<string, object>>());

        _cosmosStores[typeof(TDataSource).Name].GetOrAdd(tenantId, x => new ConcurrentDictionary<string, object>());

        var store = _cosmosStores[typeof(TDataSource).Name][tenantId].GetOrAdd(typeof(TEntity).Name, x => new CosmosStore<TEntity>(dataSource, tenantId));

        return (CosmosStore<TEntity>)store;
    }
}

`

Where tenantId = collectionName

espray commented 4 years ago

@mortana89 I am working on a multi-tenant app with a container per tenant.

Are you provisioning throughput at the database level or container level? If provisioning throughput at the database level, how are you handling a noise tenant?