Skip to content
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

Implement Redis distributed cache without Lua #54689

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Mar 22, 2024

Implement Redis distributed cache without Lua

Resolves #54685

This uses the same logic, but moves the logic from Lua to pipelined regular Redis commands - the same commands. This is more efficient, and avoids problems when the server is not Lua-capable.

Note that the pipeline code needs some commentary:

  • for the async path it uses two overlapped async operations, observed via Task.WhenAll
  • for the sync path, it uses the "batch" API in SE.Redis, which allows us to synchronously wait for multiple commands sent as though they were async, but they should be completed when it returns; hence we just check the results

Additional changes:

  • stabilize tests - .NET 9 seems to have different test concurrency rules, so: added key-isolation per test
  • added async expiration tests; two different code paths deserve two sets of tests

The tests are not "live" in CI, but work locally:

Build succeeded in 18.7s
Test run succeeded. Total: 98 Failed: 0 Passed: 98 Skipped: 0, Duration: 17.6s

(tests take this long because of the problems of using an external database for expiration)

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Mar 22, 2024
@mgravell mgravell added the feature-caching Includes: StackExchangeRedis and SqlServer distributed caches label Mar 22, 2024
@mgravell mgravell marked this pull request as ready for review March 22, 2024 14:00
@mgravell
Copy link
Member Author

/asp run

@mgravell mgravell force-pushed the marc/no-lua-redis branch from ef5512b to 77453f1 Compare March 26, 2024 14:42
@mgravell
Copy link
Member Author

(rebased in a wild attempt to fix the E2E failures)

@badrishc
Copy link

I'm concerned that simply enabling and depending on client side pipelining is going to lose atomicity guarantee for multi-threaded cache servers like garnet and dragonfly. Just because the commands are pipelined doesn't guarantee they execute as a transaction in the server side. It needs to be written as multi/exec.

@mgravell
Copy link
Member Author

mgravell commented Mar 26, 2024

@badrishc if you feel strongly that multi/exec is warranted: I'm fine with doing the work to do that

but ... just sayin' ... if Garnet happened to implement a custom command that was the logical equivalent of the hypothetical hsetex ... I'd totally use it if I detected Garnet :)

consider:

SOMECOMMAND key [FLAGS] seconds field value [field value ...]

that was a server-side equivalent of

HSET key field value [field value ...]
EXPIRE key seconds [FLAGS]

However, I won't hold my breath on that...

@badrishc
Copy link

@badrishc if you feel strongly that multi/exec is warranted: I'm fine with doing the work to do that

but ... just sayin' ... if Garnet happened to implement a custom command that was the logical equivalent of the hypothetical hsetex ... I'd totally use it if I detected Garnet :)

consider:

SOMECOMMAND key [FLAGS] seconds field value [field value ...]

that was a server-side equivalent of

HSET key field value [field value ...]
EXPIRE key seconds [FLAGS]

However, I won't hold my breath on that...

Per our discussion, it seems fine to go ahead with your solution. Would rather avoid multi/exec if customers aren't asking for the atomicity. Thanks.

@mgravell
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mgravell mgravell enabled auto-merge (squash) March 28, 2024 18:12
@EngRajabi
Copy link

@mgravell
It is not best to implement a new implementation of IDistributedCache for Garnet instead of matching the IDistributedCache implemented for Redis with Garnet.
This has good advantages. In the future, if we want to use the special features of Garnet, our hands are open

@mgravell
Copy link
Member Author

mgravell commented Mar 29, 2024

@EngRajabi we can cross that bridge trivially if we get to it; today: there would be zero differences other than "don't use Lua", which this PR harmonizes for both. We also have the ability to use server-detection in here to pick and choose features - we used to do that to choose hset vs hmset depending on the redis_version, for example - so even a single cache implementation has the capability to use implementation-specific features. Ultimately, SE.Redis is a RESP client; it doesn't actually care what the server is, as long as it can talk RESP; and both Redis and Garnet (and a bunch of others) are RESP servers.

Our hands are already: fully open.

@mgravell mgravell merged commit 1595d50 into main Apr 4, 2024
26 checks passed
@mgravell mgravell deleted the marc/no-lua-redis branch April 4, 2024 17:11
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-caching Includes: StackExchangeRedis and SqlServer distributed caches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redis distributed cache does not need to use Lua
4 participants