aspnet / Caching

[Archived] Libraries for in-memory caching and distributed caching. Project moved to https://github.com/aspnet/Extensions
Apache License 2.0
480 stars 217 forks source link

IDistributedCache should include StringSet/SetString #169

Closed shanselman closed 8 years ago

shanselman commented 8 years ago

Right now IDistributedCache is a generic interface that's presumably meant to support lots of distributed caches. It's pretty clear, though, that Redis is going to be 80% or more of the usage. Not to mention Azure has a Redis As A Service.

IDistributedCache has SetAsync which just takes bytes...as it is there's no easy way to add overloads/extensions to get at the underlying Redis implementation and set strings.

As we are today, you can't set from ASP.NET Core and GET from the CLI. redis-cli supports SET and GET with strings. This makes debugging with the CLI a hassle.

GetString and SetString are atomic enough that they should be added to IDistributedCache.

/cc @nickcraver @mgravell @DamianEdwards @rustd

Tratcher commented 8 years ago

It's worth noting that we don't just store the bytes, we store our own complex object containing the expiration information: https://github.com/aspnet/Caching/blob/dev/src/Microsoft.Extensions.Caching.Redis/RedisCache.cs#L81-L88 So even if we added a string version, I don't know if you'd be able to view it from the CLI. Have you tried to hack it in yet?

mgravell commented 8 years ago

Hacky but possibly workable:

static Task SetAsync(this IDistributedCache cache, string key, string value, DistributedCacheEntryOptions options) {
    return cache.SetAsync(key, Encoding.UTF8.GetBytes(value), options);
}

On the CLI front: looking at that script, as long as the caller knows to use HGETALL {key} rather than GET {key}, that should actually be usable and readable. Unlike our even hackier in-house middleware at stack that is actually a single blob (per key) of a protobuf-encoded object that (like you) has optional sliding expiration and may or may not have the payload gzip compressed!

As a side note, I so want to C#6-ify that RedisCache.cs linked above...

    private const string SetScript = $@"
            redis.call('HMSET', KEYS[1], '{AbsoluteExpirationKey}', ARGV[1], '{SlidingExpirationKey}', ARGV[2], '{DataKey}', ARGV[4])
            if ARGV[3] ~= '{NotPresent}' then
              redis.call('EXPIRE', KEYS[1], ARGV[3])
            end
            return 1";

;p

shanselman commented 8 years ago

Thanks Marc...I guess after "the basics" everyone starts piggybacking metadata on top or putting it in the serialization format, don't they?

HGETALL works for my basic learning/teaching scenario, thanks!

On Tue, Mar 22, 2016 at 2:51 PM, Marc Gravell notifications@github.com wrote:

Hacky but possibly workable:

static Task SetAsync(this IDistributedCache cache, string key, string value) { return cache.SetAsync(key, Encoding.UTF8.GetBytes(value)); }

On the CLI front: looking at that script, as long as the caller knows to use HGETALL {key} rather than GET {key}, that should actually be usable and readable. Unlike our even hackier in-house middleware that is actually a single blob of a protobuf-encoded object that (like you) has optional sliding expiration and may or may not have the payload gzip compressed!

As a side note, I so want to C#6-ify that RedisCache.cs linked above...

private const string SetScript = $@"
        redis.call('HMSET', KEYS[1], '{AbsoluteExpirationKey}', ARGV[1], '{SlidingExpirationKey}', ARGV[2], '{DataKey }', ARGV[4])
        if ARGV[3] ~= '{NotPresent }' then
          redis.call('EXPIRE', KEYS[1], ARGV[3])
        end
        return 1";

;p

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/aspnet/Caching/issues/169#issuecomment-200046760

Scott Hanselman Donate to fight diabetes: http://hnsl.mn/fightdiabetes

muratg commented 8 years ago

Putting this to 1.0.0 so that we can reconsider after RC2 ships

RehanSaeed commented 8 years ago

I wrote a full set of extension methods for IDistributedCache which includes Get/Set String. You may also want to consider Get/Set anything of type T (Serializes/Deserializes T to/from JSON and stores it as a string) which I also wrote. It can be found:

See also https://github.com/aspnet/Caching/pull/94

sebastienros commented 8 years ago

If think this should just be an extension method as Marc is suggesting and not at the interface level. The result is the same in the end for the end user. Having a set of primitive conversions like Rehan has done would be nice too.

Grauenwolf commented 8 years ago

If it is at the interface level, the cache provider can control how serialization works. For example, the cache may prefer to represent the data in XML or JSON format instead of binary.

basitanwer commented 8 years ago

Redis is going to be 80% or more of the usage

While this maybe true at the moment but nobody can predict the future. For a specific use case we can't/shouldn't change an interface which is an abstraction.

ChrisProlls commented 8 years ago

Just by curiosity, why IDistributedCache has no extensions methods for storing objects ? Something like Get(string key) or Set(string key, Type value) ?

Tratcher commented 8 years ago

@ChrisProlls just because we haven't needed them yet. Session is one of the only consumers at the moment and it uses it's own serialization logic.

Grauenwolf commented 8 years ago

Another use case is two-level caching.

Some caches may want to keep a subset of your object in a local cache so that it can quickly give it back to you.

The current design requires a potentially expensive deserialization process even if the object doesn't need to be fetched over the wire.

Grauenwolf commented 8 years ago

I don't see this interface as an abstraction at all, and that's the problem.

Instead it is specifically written for caches that use binary as their communication protocol and for cache consumers that want to control serialization.

We need a generic contract for distributed caches that can be used beyond that very narrow scenario and this isn't it.

sebastienros commented 8 years ago

@Grauenwolf This specific interface is not designed to support your scenario obviously, and that's fine. I suggest you create a custom service that could reuse IDistributedCache and IMemoryCache to fulfill what you need. The implementation would only have the fallback logic you are mentioning, so it should be pretty simple. Having a service that does all the things anyone could require would be a bigger problem IMO.

basitanwer commented 8 years ago

I agree to @Grauenwolf and sort of with @shanselman as well

void Set(string key, byte[] value, DistributedCacheEntryOptions options);

is very specific. While I would suggest the value should not be string but would also agree that it should be something other than a byte array. Would the type object suffice here ?

The better implementation IMHO would be to create a custom object and make it implicit. A similar implementation to RedisValue. https://github.com/StackExchange/StackExchange.Redis/blob/master/StackExchange.Redis/StackExchange/Redis/RedisValue.cs

muratg commented 8 years ago

@basitanwer How would the recommended method serialize a random object?

Grauenwolf commented 8 years ago

If this interface isn't meant to be a generic caching abstraction then it shouldn't be called "Microsoft.Extensions.Caching". Call it "Microsoft.Extensions.CachingForAspOnly" so that its purpose is clear.

And then start a project so that we have a real generic caching framework.

Grauenwolf commented 8 years ago

As for serializing generic objects, that would be adapter specific. But in general, it should default to using DataContract attributes and the associated SOAP or JSON serializer.

And that's part of my point. Serialization should be an infrastructure concern and not bleed into the application code. Otherwise it isn't really an abstraction.

Grauenwolf commented 8 years ago

I suggest you create a custom service that could reuse IDistributedCache and IMemoryCache to fulfill what you need.

That's not really possible. Once the object is a binary array it becomes a black box. So implementing the interface won't work.

I could layer my own interfaces above those two, but then we're back to everyone making up their own, incompatible interface. And the whole point of having an interface is that everyone follows the same contract.

basitanwer commented 8 years ago

@muratg : @Grauenwolf cleared my point. In case of objects either the default .NET serialize should be called or any custom. Under any circumstance the serialization should be part of the implementation/adapter not the application.

ASP.NET Caching if provides a caching framework where you can easily plug in different distributed caches then the framework should be generic not specific to Redis or SQL or NCache or another for that matter.

muratg commented 8 years ago

@basitanwer What do you mean by "default .NET serialize"? You can't serialize arbitrary objects. Object itself and all the objects in its graph has to be have Serializable attribute, or you have to provide a custom serializer.

Grauenwolf commented 8 years ago

Nobody uses the Serializable attribute anymore. The DataContract serializer can use DataContract/DataMember attributes, but it isn't required. You just need a clear tree structure without loops.

Other serializers such as JSON use the same patterns and optional attributes.

muratg commented 8 years ago

@Grauenwolf You also need a parameterless constructor. And if you don't use the attributes, only public surface would be serialized.

Grauenwolf commented 8 years ago

While that is true, it is also a well-known limitation. Anyone who uses WCF or ASP.NET WebAPI should already be very familiar with those rules and their implications.

Eilon commented 8 years ago

@BrennanConroy looks like we just need to add Get/SetString.

Eilon commented 8 years ago

BTW these should be extension methods on IDistributedCache, not methods on the interface.

Tratcher commented 8 years ago

@BrennanConroy These can be copied from Session and go in Caching.Abstractions.

BrennanConroy commented 8 years ago

https://github.com/aspnet/Caching/commit/4e750c65d6d2b86bdf339cdf50ba815787e4c312