Fix RedisCache update that breaks usage against older redis servers #38860
Conversation
ghost
added
the
community-contribution
| private const string SlidingExpirationKey = "sldexp"; | ||
| private const string DataKey = "data"; | ||
| private const long NotPresent = -1; | ||
| private readonly Version ServerVersionWithExtendedSetCommand = new Version(4, 0, 0); |
There was a problem hiding this comment.
| private readonly Version ServerVersionWithExtendedSetCommand = new Version(4, 0, 0); | |
| private static readonly Version ServerVersionWithExtendedSetCommand = new Version(4, 0, 0); |
Maybe?
| // ARGV[4] = data - byte[] | ||
| // this order should not change LUA script depends on it | ||
| private const string SetScript = (@" | ||
| private const string RegularSetScript = (@" |
There was a problem hiding this comment.
| private const string RegularSetScript = (@" | |
| private const string LatestSetScript = (@" |
| // ARGV[4] = data - byte[] | ||
| // this order should not change LUA script depends on it | ||
| private const string SetScript = (@" | ||
| private const string LatestSetScript = (@" |
There was a problem hiding this comment.
Nit: Consider names other than "Latest..." and "Deprecated...". Maybe "SetScript_HsetBased" and "SetScript_HmsetBased" or something like that?
There was a problem hiding this comment.
Nothing good comes to mind :(
Imho, nearby comment, explaining why two types of SetScript are used, helps a lot.
There was a problem hiding this comment.
FWIW I'd name this aligned with the property, e.g. SetScriptPreExtendedSetCommand - verbose but super clear.
There was a problem hiding this comment.
Thanks @fvoronin, this looks good to me aside from a couple of minor comments I left.
|
Thanks @fvoronin! |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1561207568 |
|
@wtgodbe backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix RedisCache update that breaks usage against older redis servers (#38715)
Applying: Suggested changes (#38715)
Applying: Suggested changes (#38715)
Applying: Explanation of why two kinds of SetScript are used
error: sha1 information is lacking or useless (src/Caching/StackExchangeRedis/src/RedisCache.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Explanation of why two kinds of SetScript are used
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
| // ARGV[4] = data - byte[] | ||
| // this order should not change LUA script depends on it | ||
| private const string SetScript = (@" | ||
| private const string SetScriptPerExtendedSetCommand = (@" |
There was a problem hiding this comment.
@NickCraver @adityamandaleeka Was this supposed to be SetScriptPreExtendedSetCommand? And was this meant to apply to what's currently called DeprecatedSetScript?
There was a problem hiding this comment.
@halter73 You're right, that's how I interpreted @NickCraver's comment too. I saw that it was renamed but didn't catch that it was applied to the newer one (in which case I guess "per" kinda makes sense 😄).
There was a problem hiding this comment.
@halter73 yep, that's what I was pitching there :)
There was a problem hiding this comment.
I'll send a quick PR to rename them to SetScript and SetScriptPreExtendedSetCommand.
There was a problem hiding this comment.
Thanks again for noticing that @halter73
There was a problem hiding this comment.
@adityamandaleeka, @halter73, thanks!
amcasey
added
the
area-networking
9 participants
Summary of the changes
Checking Redis version and then calling the right API.
A couple of questions:
Fixes #38715