Open udlose opened 3 years ago
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
I don't have write permissions but ill tag the lead for the area: @ericstj , @eerhardt
Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp See info in area-owners.md if you want to be subscribed.
Author: | udlose |
---|---|
Assignees: | - |
Labels: | `area-Extensions-Caching`, `untriaged` |
Milestone: | - |
Can adding support for an extra optional parameters to IDistributedCache interface's methods solve this ?
IDistributedCache
wouldnt be getting Redis or StackExchange related logic in. Transferring to aspnetcore repo to carry conversation forward.
@udlose Can you share more about why you need this? It seems strange to modify the interface to support implementation-specific parameters.
Hi @udlose. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.
@adityamandaleeka, @maryamariyan - perhaps the confusion is that I misspoke by asking that the implementation-agnostic interface of IDistributedCache
be modified to include implementation-specific parameters pertaining to [StackExchange.Redis.CommandFlag]
(https://github.com/StackExchange/StackExchange.Redis/blob/8612fb8a9278822c87d5476819325a7438e596ca/src/StackExchange.Redis/Enums/CommandFlags.cs#L47).
I've updated the title and feature request details to help clarify.
To rephrase, for performance reasons, I'd like to be able to specify whether my Redis API call is sent to a Replica, Slave, or Master instance. As I mention, the Microsoft.Extensions.Caching.StackExchangeRedis.RedisCache
implementation doesn't allow for specifying these valuable params. So at a minimum, I think that ability should be added. It would at least give users the ability to create an interface (e.g. IDistributedRedisCache
) that exposes those params on the API interface. Though I'd argue that the IDistributedRedisCache
interface should be implemented in the Microsoft.Extensions.Caching.StackExchangeRedis
nuget and not place the responsibility on devs to have to create the interface. IMO, an API should be able to be consumed without additional work on behalf of the API consumer.
I am just throwing out an idea of what the implementation might look like. If you have an alternative that your more amenable to but still provides the ability to specify the [StackExchange.Redis.CommandFlag]
, I'd be happy just the same.
Please let me know if you have any more questions as I'd really like to see this added. It seems important from a performance perspective.
Thanks @udlose. Adding this to the RedisCache implementation seems reasonable.
We don't want to create another new interface but as you mentioned, users can create that if they want it.
We'd take a contribution to add this if you're interested. Feel free to copy/paste the issue template here into a comment on this issue and we will review it.
We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.
Thanks @udlose. Adding this to the RedisCache implementation seems reasonable.
We don't want to create another new interface but as you mentioned, users can create that if they want it.
@adityamandaleeka why are you not open to adding this interface to the Microsoft.Extensions.Caching.StackExchangeRedis
nuget?
To clarify, we are open to updating Microsoft.Extensions.Caching.StackExchangeRedis.RedisCache
with the added parameter. The thing we don't think is useful is creating and consuming another new interface like IDistributedRedisCache.
@adityamandaleeka, @maryamariyan so I'm working on submitting a PR for this change and I have a question. I'm making changes as follows to Microsoft.Extensions.Caching.StackExchangeRedis.RedisCache
:
// THE METHOD BELOW IS EXISTING
/// <inheritdoc />
public byte[]? Get(string key)
{
if (key == null)
{
throw new ArgumentNullException(nameof(key));
}
return GetAndRefresh(key, getData: true);
}
// THE METHOD BELOW IS NEW
/// <inheritdoc />
public byte[]? Get(string key, CommandFlags commandFlags)
{
if (key == null)
{
throw new ArgumentNullException(nameof(key));
}
return GetAndRefresh(key, getData: true, commandFlags: commandFlags);
}
// THE METHOD BELOW IS EXISTING
/// <inheritdoc />
public async Task<byte[]?> GetAsync(string key, CancellationToken token = default(CancellationToken), CommandFlags commandFlags = CommandFlags.None)
{
//code elided
}
When I add my default params, to the end of the GetAsync
method (so that I do not violate RS0027
(https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md), I get a CompilerError: CS 1068 (CancellationToken parameters must come last).
If I have the new CommandFlag
param before the optional CancellationToken
param to satisfy CA1068, I violate binary compatibility and RS0027
. I don't want to break binary compatibility but I don't like ignoring compiler warnings either.
What are your suggestions?
Actually it looks as if I won't be able to add the CommandFlags
param to this signature because it then no longer implements the IDistributedCache
signature of:
public async Task<byte[]?> GetAsync(string key, CancellationToken token = default(CancellationToken))
If I add another signature to not break the IDistributedCache
contract, I then break RS0027 - Adding optional parameters to public methods
public async Task<byte[]?> GetAsync(string key, CommandFlags commandFlags, CancellationToken token)
Not sure where to go from here.
@adityamandaleeka @maryamariyan any updates or suggestions on how to proceed? (see above comment)
If I understand this right, since the IDistributedCache is using the IConnectionMultiplexer directly.. Perhaps a way forward would be to configure (or likely add a config option to ...) SE.Redis to PreferReplica on read operations when the multiplexer is created initially.
And/or adding a parameter to the RedisCacheOptions passed into AddStackExchangeRedisCache, specifying PreferReplica on reads..
That way IDistributedCache's interface is none the wiser, but the underlying implementation can direct reads/writes appropriately.
Could that work?
If I understand this right, since the IDistributedCache is using the IConnectionMultiplexer directly.. Perhaps a way forward would be to configure (or likely add a config option to ...) SE.Redis to PreferReplica on read operations when the multiplexer is created initially.
And/or adding a parameter to the RedisCacheOptions passed into AddStackExchangeRedisCache, specifying PreferReplica on reads..
That way IDistributedCache's interface is none the wiser, but the underlying implementation can direct reads/writes appropriately.
Could that work?
At a minimum, that would provide at least some way to customize the behavior. It would be nice to be able to have flexibility to change behavior via API parameterss as the StackExchangeRedis API's have.
You can't change the interface (it's a breaking change), you also can't add redis specific APIs to the base interface, it would break the abstraction. It seems like you want a redis interface, IDistributedCache isn't it.
You can't change the interface (it's a breaking change), you also can't add redis specific APIs to the base interface, it would break the abstraction. It seems like you want a redis interface, IDistributedCache isn't it.
Not sure which "You" that you're replying to here, so your message is ambiguous.
My bad, I was looking referring to this comment. My read of the issue is that we're trying to flow a per call (Get/Set) flag from IDistributedCache to the underlying redis implementation and that breaks the abstraction.
It seems like there's a desire for a more lightweight redis specific interface.
There's an awkward workaround to use the factory option of redis distributed cache implementation to return a decorated connection multiplexer that returns decorated redis clients that can then take hints from the key name and set the appropriate flag...
Awkward and tedious but would work.
This is also causing us grief. All traffic is going to the writer and no load is able to go to the read replicas. Instead of modifying the interface, what if the CommandFlags are passed in the options during registration, then used by the RedisCache implementation?
In RedisCacheOptions.cs
add public CommandFlags ReadCommandFlags { get; set; }
.
In RedisCache.GetAndRefresh
and RedisCache.GetAndRefreshAsync
add _options.ReadCommandFlags
to the cache.HashGet
call.
The scope should be pretty small. Existing uses won't be impacted - the default CommandFlags value is None, matching the HashGet's default value. Users that want to use read replicas can set RedisCacheOptions.ReadCommandFlags = CommandFlags.PreferReplica
.
This is a pain point for us as well. The changes that @Mindex-Brandon-Key suggested would fit our use case as well.
I just found https://github.com/dotnet/aspnetcore/issues/28375 which seems to echo the missing behavior
Is your feature request related to a problem? Please describe.
I'd like to be able to specify the usage of a Replica or Slave endpoint instead of always going to the Master. This would allow me to offload traffic for read-only cache operations where the latest data is not critical resulting in improved performance.
Currently, the
StackExchange.Redis.IDatabase
API allows for this by use of optionalStackExchange.Redis.CommandFlag
enum that can be specified in API calls. However, the implementation ofMicrosoft.Extensions.Caching.StackExchangeRedis.RedisCache
doesn't provide these optional params to be passed thru.Describe the solution you'd like
I would like for the implementation of
Microsoft.Extensions.Caching.StackExchangeRedis.RedisCache
to provide the ability to pass a flag that allows me to specify replica usage (master, slave, replica, etc.). You would probably need to create a new interface to expose this whichMicrosoft.Extensions.Caching.StackExchangeRedis.RedisCache
would implement e.g.IDistributedRedisCache
.Describe alternatives you've considered
Using
StackExchange.Redis.IDatabase
directly instead of Microsoft'sIDistributedCache
wrapper. Unfortunately, it would be a lot of work to change for our applications and we do prefer theIDistributedCache
wrapper in general. We also use this with Polly for caching and Polly provides a nice integration withIDistributedCache
already.