-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix garnet persistence #5087
Fix garnet persistence #5087
Conversation
|
aspire/src/Aspire.Hosting.Garnet/GarnetBuilderExtensions.cs Lines 137 to 139 in f42201f
also despite I want to write functional tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking the existing API as Obsolete and introducing a new API sounds like the right path to me.
@eerhardt, do you agree with the last change? |
@@ -83,7 +83,7 @@ public static class GarnetBuilderExtensions | |||
isReadOnly); | |||
if (!isReadOnly) | |||
{ | |||
builder.WithPersistence(); | |||
builder.WithPersistence(interval: null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate that the overloads are conflicting. Can we solve this somehow? Potentially by making the Obsolete overload not have default parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt, do you mean something like this?
[Obsolete("This method is obsolete and will be removed in a future version. Use the overload without the keysChangedThreshold parameter.")]
public static IResourceBuilder<GarnetResource> WithPersistence(this IResourceBuilder<GarnetResource> builder,
TimeSpan? interval, long keysChangedThreshold)
=> WithPersistence(builder, interval);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think removing the optional parameters on the Obsolete overload is a good compromise here.
@mitchdenny - thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the strategy.
Looks like the new test is currently failing. |
So, if I am following all of this correctly the following happened:
I think I'm OK with the breaking change here - it seems necessary. |
Actually, we used the wrong args plus they also had issues in persistance. |
|
Looks like the non-volume test is broken:
|
There is issue in the Linux host, this test works in my local (Windows host)
|
@eerhardt the test still failing |
I fixed it locally on my linux machine. It should be passing now. |
I don't think we need a garnet sample until we have a garnet client. Can we add garnet and valkey to the same sample? |
I could add garnet to the redis sample in this PR and valkey in another PR. |
@davidfowl, This PR is ready to merge. If you merge it, I can add the Valkey sample to the playground app. |
Thanks @Alirexaa |
Fixes: #4870
Microsoft Reviewers: Open in CodeFlow