Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Remove Connect and ConnectAsync methods from IDistributedCache interface #135

Closed
kichalla opened this issue Dec 28, 2015 · 8 comments
Closed
Assignees
Milestone

Comments

@kichalla
Copy link
Member

@lodejard brought up a good point of the need of having a Connect method on the interface. They seem to be implementation details for the particular distributed cache implementation (ex: RedisCache). SqlServerCache and LocalCache(distributed MemoryCache) do not require them.

https://github.com/aspnet/Caching/blob/dev/src/Microsoft.Extensions.Caching.Abstractions/IDistributedCache.cs#L10-L12

cc @Tratcher @muratg @Eilon @davidfowl

And regarding related issue #131, RedisCache can implement an IDisposable interface and in the dispose method can close the connections. Currently RedisCache is registered as singleton and the DI would call the Dispose method on the singleton instance(I verified this is happening)

@kichalla
Copy link
Member Author

I have the following PR with the changes
#136

@muratg muratg added this to the 1.0.0-rc2 milestone Dec 28, 2015
@muratg muratg assigned muratg and kichalla and unassigned muratg Dec 28, 2015
@Eilon Eilon assigned lodejard and unassigned kichalla Dec 31, 2015
@Eilon
Copy link
Member

Eilon commented Dec 31, 2015

@lodejard putting on your plate to review @kichalla 's PR at #136 and merge.

@basitanwer
Copy link

Guys why are we removing the Connect methods from IDistributedCache ?

I understand that SqlServerCache and LocalCache(distributed MemoryCache) do not require them but in reality they are not distributed caches.

Redis, NCache or any other distributed caches are very different and support different needs and they do not provided a connection pool therefore a Connect/Initialize method is a must.

@sebastienros
Copy link
Member

@basitanwer If you look at the PR you will see how it works for Redis too. NCache won't be different. You can still do your own pool if necessary.

@Tratcher Tratcher assigned Tratcher and unassigned lodejard Jan 25, 2016
@aggieben
Copy link

aggieben commented Jul 23, 2016

I read the PR, and I think I understand it. But I don't think the response to @basitanwer is complete.

Because connections are now done as a part of every Get and Set (and async variants) operation, how does one now handle failure scenarios? I.e., when a Get gives you a null result, how do you know when that's because the key doesn't exist in the cache and when it's because the cache isn't even there?

Just to close the loop, how would you suggest it would be possible to implement a connection pool without at least rewriting RedisCache?

@Tratcher
Copy link
Member

@aggieben either way it's a cache-miss. Your app should function even when the cache is unavailable, even if less efficiently.

Connection pooling, if any, should happen inside the cache provider, that's not a detail that should be exposed the the application.

@aggieben
Copy link

aggieben commented Aug 14, 2016

@Tratcher You're not wrong, but as far as I can tell there are legitimate concerns here that haven't yet been provided responses. Firstly, the official answer to the OP was "look at the PR, you can still do your own pool" - but I'm pointing out that it's non-obvious how that can be done without rewriting RedisCache and still haven't really gotten a response to that.

Secondly, and more importantly for me personally, we need to know what kind of cache miss so that we can implement fallback strategies. In my particular case, if the redis isn't even there, then I fallback to a different caching strategy. With these changes it is now impossible to correctly implement such a thing without having to re-write code that the framework already has but is now inaccessible.

@Tratcher
Copy link
Member

If your redis server isn't there then I'd expect that to be a deployment / configuration error, not something you'd try to deal with at runtime.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants