Common code for caching systems (Draft)#4729
Common code for caching systems (Draft)#4729Zombach wants to merge 12 commits intomicrosoft:mainfrom
Conversation
ghost
added
the
area-codeflow
dotnet-policy-service
bot
added
the
community-contribution
|
@davidfowl Hi, could you please rate this? |
|
Is this common to caching systems or is it common to systems supporting Redis' protocol? |
Currently systems supporting the Redis protocol |
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=725441&view=codecoverage-tab |
|
@Zombach I think that this is too big of a change (there are breaking changes here). We'd probably want to spin up an API proposal around this so we could get into the motivations and shape of API changes here before we get too much futher. Obviously across things like Redis, Garnet, and Valkey there is a common protocol (currently) but I am not 100% sure that means that we should bind them together this way. |
+1 |
danmoseley
added
area-integrations
I understand that the change is large and complex, but I made it big on purpose to test whether all the cases would pass the test. Or there will be some problems. But as I was convinced, everything works, and the tests passed. As for the API, let's first focus on the option without RedisCommander. I’ll write using Garnet as an example, it’s clear that for the Redis protocol everything is identical At the moment, we are using three classes. ///This class contains constants
internal static class GarnetContainerImageTags
{
public const string Registry = "ghcr.io";
public const string Image = "microsoft/garnet";
public const string Tag = "1.0";
}I believe that GarnetContainerImageTags class should be created for each protocol, as it is now. public class GarnetResource(string name) : ContainerResource(name), IResourceWithConnectionString
{
internal const string PrimaryEndpointName = "tcp";
private EndpointReference? _primaryEndpoint;
/// <summary>
/// Gets the primary endpoint for the Garnet server.
/// </summary>
public EndpointReference PrimaryEndpoint => _primaryEndpoint ??= new(this, PrimaryEndpointName);
/// <summary>
/// Gets the connection string expression for the Garnet server.
/// </summary>
public ReferenceExpression ConnectionStringExpression =>
ReferenceExpression.Create(
$"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}");
}This class should be made abstract and raised to a level, and inherited from it. /// <summary>
/// A resource that represents a Garnet resource independent of the hosting model.
/// </summary>
/// <param name="name">The name of the resource.</param>
public class GarnetResource(string name) : CacheResource(name);Since RedisResource additionally uses the method. /// <summary>
/// A resource that represents a Redis resource independent of the hosting model.
/// </summary>
/// <param name="name">The name of the resource.</param>
public class RedisResource(string name) : CacheResource(name)
{
/// <summary>
/// Gets the connection string for the Redis server.
/// </summary>
/// <param name="cancellationToken"> A <see cref="CancellationToken"/> to observe while waiting for the task to complete.</param>
/// <returns>A connection string for the redis server in the form "host:port".</returns>
public ValueTask<string?> GetConnectionStringAsync(CancellationToken cancellationToken = default)
{
if (this.TryGetLastAnnotation<ConnectionStringRedirectAnnotation>(out var connectionStringAnnotation))
{
return connectionStringAnnotation.Resource.GetConnectionStringAsync(cancellationToken);
}
var referenceExpression = ReferenceExpression.Create(
$"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}");
return referenceExpression.GetValueAsync(cancellationToken);
}
}Regarding GarnetBuilderExtensions // Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System.Globalization;
using Aspire.Hosting.ApplicationModel;
using Aspire.Hosting.Garnet;
using Aspire.Hosting.Utils;
namespace Aspire.Hosting;
/// <summary>
/// Provides extension methods for adding Garnet resources to the application model.
/// </summary>
public static class GarnetBuilderExtensions
{
private const string GarnetContainerDataDirectory = "/data";
/// <summary>
/// Adds a Garnet container to the application model.
/// </summary>
/// <example>
/// Use in application host
/// <code lang="csharp">
/// var builder = DistributedApplication.CreateBuilder(args);
///
/// var garnet = builder.AddGarnet("garnet");
/// var api = builder.AddProject<Projects.Api>("api)
/// .WithReference(garnet);
///
/// builder.Build().Run();
/// </code>
/// </example>
/// <example>
/// Use in Api with Aspire.StackExchange.Redis
/// <code lang="csharp">
/// var builder = WebApplication.CreateBuilder(args);
/// builder.AddRedisClient("garnet");
///
/// var multiplexer = builder.Services.BuildServiceProvider()
/// .GetRequiredService<IConnectionMultiplexer>();
///
/// var db = multiplexer.GetDatabase();
/// db.HashSet("key", [new HashEntry("hash", "value")]);
/// var value = db.HashGet("key", "hash");
/// </code>
/// </example>
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/>.</param>
/// <param name="name">The name of the resource. This name will be used as the connection string name when referenced in a dependency.</param>
/// <param name="port">The host port to bind the underlying container to.</param>
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<GarnetResource> AddGarnet(this IDistributedApplicationBuilder builder, string name,
int? port = null)
{
var garnet = new GarnetResource(name);
return builder.AddResource(garnet)
.WithEndpoint(port: port, targetPort: 6379, name: GarnetResource.PrimaryEndpointName)
.WithImage(GarnetContainerImageTags.Image, GarnetContainerImageTags.Tag)
.WithImageRegistry(GarnetContainerImageTags.Registry);
}
/// <summary>
/// Adds a named volume for the data folder to a Garnet container resource and enables Garnet persistence.
/// </summary>
/// <example>
/// Use <see cref="WithPersistence(IResourceBuilder{GarnetResource}, TimeSpan?, long)"/> to adjust Garnet persistence configuration, e.g.:
/// <code lang="csharp">
/// var cache = builder.AddGarnet("cache")
/// .WithDataVolume()
/// .WithPersistence(TimeSpan.FromSeconds(10), 5);
/// </code>
/// </example>
/// <param name="builder">The resource builder.</param>
/// <param name="name">The name of the volume. Defaults to an auto-generated name based on the application and resource names.</param>
/// <param name="isReadOnly">
/// A flag that indicates if this is a read-only volume. Setting this to <c>true</c> will disable Garnet persistence.<br/>
/// Defaults to <c>false</c>.
/// </param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<GarnetResource> WithDataVolume(this IResourceBuilder<GarnetResource> builder,
string? name = null, bool isReadOnly = false)
{
builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), GarnetContainerDataDirectory,
isReadOnly);
if (!isReadOnly)
{
builder.WithPersistence();
}
return builder;
}
/// <summary>
/// Adds a bind mount for the data folder to a Garnet container resource and enables Garnet persistence.
/// </summary>
/// <example>
/// Use <see cref="WithPersistence(IResourceBuilder{GarnetResource}, TimeSpan?, long)"/> to adjust Garnet persistence configuration, e.g.:
/// <code lang="csharp">
/// var garnet = builder.AddGarnet("garnet")
/// .WithDataBindMount()
/// .WithPersistence(TimeSpan.FromSeconds(10), 5);
/// </code>
/// </example>
/// <param name="builder">The resource builder.</param>
/// <param name="source">The source directory on the host to mount into the container.</param>
/// <param name="isReadOnly">
/// A flag that indicates if this is a read-only mount. Setting this to <c>true</c> will disable Garnet persistence.<br/>
/// Defaults to <c>false</c>.
/// </param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<GarnetResource> WithDataBindMount(this IResourceBuilder<GarnetResource> builder,
string source, bool isReadOnly = false)
{
builder.WithBindMount(source, GarnetContainerDataDirectory, isReadOnly);
if (!isReadOnly)
{
builder.WithPersistence();
}
return builder;
}
/// <summary>
/// Configures a Garnet container resource for persistence.
/// </summary>
/// <example>
/// Use with <see cref="WithDataBindMount(IResourceBuilder{GarnetResource}, string, bool)"/>
/// or <see cref="WithDataVolume(IResourceBuilder{GarnetResource}, string?, bool)"/> to persist Garnet data across sessions with custom persistence configuration, e.g.:
/// <code lang="csharp">
/// var cache = builder.AddGarnet("cache")
/// .WithDataVolume()
/// .WithPersistence(TimeSpan.FromSeconds(10), 5);
/// </code>
/// </example>
/// <param name="builder">The resource builder.</param>
/// <param name="interval">The interval between snapshot exports. Defaults to 60 seconds.</param>
/// <param name="keysChangedThreshold">The number of key change operations required to trigger a snapshot at the interval. Defaults to 1.</param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<GarnetResource> WithPersistence(this IResourceBuilder<GarnetResource> builder,
TimeSpan? interval = null, long keysChangedThreshold = 1)
=> builder.WithAnnotation(new CommandLineArgsCallbackAnnotation(context =>
{
context.Args.Add("--save");
context.Args.Add((interval ?? TimeSpan.FromSeconds(60)).TotalSeconds.ToString(CultureInfo.InvariantCulture));
context.Args.Add(keysChangedThreshold.ToString(CultureInfo.InvariantCulture));
return Task.CompletedTask;
}), ResourceAnnotationMutationBehavior.Replace);
}In this case, we also raise all methods to the main one, but modify them a little. public static IResourceBuilder<T> AddCache<T>(this IDistributedApplicationBuilder builder,
T cacheResource,
string registry,
string image,
string tag,
int targetPort,
int? port = null)
where T : CacheResource
=> builder.AddResource(cacheResource)
.WithEndpoint(port: port, targetPort: targetPort, name: CacheResource.PrimaryEndpointName)
.WithImage(image, tag)
.WithImageRegistry(registry);
public static IResourceBuilder<T> WithDataVolume<T>(this IResourceBuilder<T> builder,
string target,
string? name = null,
bool isReadOnly = false)
where T : CacheResource
{
builder.WithVolume(name ?? VolumeNameGenerator.CreateVolumeName(builder, "data"), target,
isReadOnly);
if (!isReadOnly)
{
builder.WithPersistence();
}
return builder;
}
public static IResourceBuilder<T> WithDataBindMount<T>(this IResourceBuilder<T> builder,
string source,
string target,
bool isReadOnly = false)
where T : CacheResource
{
builder.WithBindMount(source, target, isReadOnly);
if (!isReadOnly)
{
builder.WithPersistence();
}
return builder;
}
public static IResourceBuilder<T> WithPersistence<T>(this IResourceBuilder<T> builder,
TimeSpan? interval = null,
long keysChangedThreshold = 1)
where T : CacheResource
=> builder.WithAnnotation(new CommandLineArgsCallbackAnnotation(context =>
{
context.Args.Add("--save");
context.Args.Add((interval ?? TimeSpan.FromSeconds(60)).TotalSeconds.ToString(CultureInfo.InvariantCulture));
context.Args.Add(keysChangedThreshold.ToString(CultureInfo.InvariantCulture));
return Task.CompletedTask;
}), ResourceAnnotationMutationBehavior.Replace);And finally, in each Redis protocol cache. we can use this kind of class. public static IResourceBuilder<GarnetResource> AddGarnet(this IDistributedApplicationBuilder builder,
string name,
int? port = null)
{
var garnetResource = new GarnetResource(name);
return builder.AddCache(garnetResource,
GarnetContainerImageTags.Registry,
GarnetContainerImageTags.Image,
GarnetContainerImageTags.Tag,
6379,
port);
}
public static IResourceBuilder<GarnetResource> WithDataVolume(this IResourceBuilder<GarnetResource> builder,
string? name = null,
bool isReadOnly = false)
=> builder.WithDataVolume(GarnetContainerDataDirectory, name, isReadOnly);
public static IResourceBuilder<GarnetResource> WithDataBindMount(this IResourceBuilder<GarnetResource> builder,
string source,
bool isReadOnly = false)
=> builder.WithDataBindMount(source, GarnetContainerDataDirectory, isReadOnly);
public static IResourceBuilder<GarnetResource> WithPersistence(this IResourceBuilder<GarnetResource> builder,
TimeSpan? interval = null,
long keysChangedThreshold = 1)
=> CacheBuilderExtensions.WithPersistence(builder, interval, keysChangedThreshold);I see it all roughly like this. If you need any more examples, please let me know as well. If you need examples specifically about the API, then please provide at least some example so that I can understand in what form to provide them to you. Thank you for your time! |
|
@Zombach thanks for the detailed response. Even though Garnet, Redis, and Valkey (and some other spinoffs from Redis) all implement the same protocol (not 100% but close enough for this conversation) the way that the containers are configured differ. My guess is that container configuration is the thing that is most likely to diverge when it comes to these Redis-like cache APIs. Given this I'm not sure it makes sense to try and converge the hosting packages in the way that you have here because once bonded together it can become more difficult to unpick down the track. A small amount of duplicated code now provides some options when the (in my opinion) inevitable happens. |
|
In #4519 (comment), @davidfowl says:
What he meant is that for common, internal code we can compile the same code into multiple libraries, so we don't need the duplication. For example, https://github.com/dotnet/aspire/blob/main/src/Shared/VolumeNameGenerator.cs is compiled into a few different libraries: Sharing internal code doesn't need any change to public API. We can leave the public API as it is today, but underneath share code across all 3 libraries. |
|
Cc @mgravell |
|
Coming in a little cold, but to me the key factor would be how much of the relevant code here is infrastructure/hosting, vs how much is runtime/monitoring. The latter: makes a lot of sense to share (and if the ship hasn't already sailed, I might suggest leaning on Resp/RESP nomenclature rather than Redis nomenclature). But @mitchdenny hints that a significant portion is the hosting, which will absolutely be bespoke per provider (although Redis and ValKey obviously have a lot of overlap, Garnet and all of the other red-ish backends will be much more varied). |
Now I got it right. Thanks for the explanation. |
You are absolutely right and I completely agree with you. I'll still try to combine the common part, but as said @davidfowl and @eerhardt. If it doesn’t work out, then I’ll just close this PR. Thanks again everyone! |
Yes, thanks for the comment, in general I understand it. But I don’t yet have enough experience to see this in advance and not take unnecessary actions. From my side. I can only say one thing. Thank you @mitchdenny and @eerhardt for bearing with me and sharing your experiences. |
|
No ... thank-you for contributing to the Aspire code base! |
|
Thanks for the contribution, @Zombach. I'm going to close this PR now as it is stale, and I don't think we want to make this change. |
7 participants
Common code for caching systems
Redis
Garnet
Valkey
And others
In connection with this task #4519 Add redis commander
Idea based on this post #4519 message
Microsoft Reviewers: Open in CodeFlow