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

feat: add Valkey as a native store #3892

Merged
merged 3 commits into from
Dec 15, 2024

Conversation

ftsartek
Copy link
Contributor

@ftsartek ftsartek commented Dec 7, 2024

Description

  • Adds Valkey as a native store alternative to Redis.
  • Adds an optional Litestar install dependency: litestar[valkey] which installs valkey with libvalkey as an optimisation layer.

@ftsartek ftsartek requested review from a team as code owners December 7, 2024 08:26
@github-actions github-actions bot added area/dependencies This PR involves changes to the dependencies area/docs This PR involves changes to the documentation area/stores size: small type/feat pr/external Triage Required 🏥 This requires triage labels Dec 7, 2024
@ftsartek ftsartek marked this pull request as draft December 7, 2024 08:38
@ftsartek ftsartek force-pushed the feat/add-valkey-native-store branch 3 times, most recently from f1adeea to b9d9137 Compare December 7, 2024 08:47
Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.35%. Comparing base (87f8aa0) to head (8c85612).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3892   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files         345      346    +1     
  Lines       15605    15660   +55     
  Branches     1725     1730    +5     
=======================================
+ Hits        15348    15403   +55     
  Misses        122      122           
  Partials      135      135           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ftsartek ftsartek force-pushed the feat/add-valkey-native-store branch from b9d9137 to 8290eda Compare December 8, 2024 06:18
@github-actions github-actions bot removed the area/docs This PR involves changes to the documentation label Dec 8, 2024
@ftsartek ftsartek force-pushed the feat/add-valkey-native-store branch 2 times, most recently from 8bd4148 to 4375467 Compare December 8, 2024 06:45
@ftsartek ftsartek marked this pull request as ready for review December 8, 2024 06:53
@ftsartek ftsartek force-pushed the feat/add-valkey-native-store branch 2 times, most recently from 8e6dc21 to fb37e94 Compare December 8, 2024 07:17
@ftsartek
Copy link
Contributor Author

ftsartek commented Dec 8, 2024

I went through and basically replicated the existing Redis tests for Valkey. They're just new tests, though I'm open to a less-duplicative approach if anyone can point me in the right direction (I guess parametrise all the currently-redis-specific tests and then add checks for type to handle client._redis vs client._valkey, etc?).

The only thing that came up is that Valkey uses server.call instead of the valkey.call analogue you might expect for Redis' redis.call, but otherwise the scripts are identical.

I'm going to leave this as it is now pending any feedback.

@github-actions github-actions bot added the area/docs This PR involves changes to the documentation label Dec 8, 2024
@ftsartek ftsartek force-pushed the feat/add-valkey-native-store branch from 8b111e4 to 992d5bc Compare December 8, 2024 07:51
Jordan Russell added 2 commits December 15, 2024 13:00
Adds support for Valkey as a store option, alternative to Redis.
Adds (very) basic testing for the Valkey store type.
Adds an autogenerated documentation page for the ValkeyStore itself.
Additionally includes some notes on the existing `stores.rst` usage page indicating the equivalence of Valkey/Redis.
@cofin cofin force-pushed the feat/add-valkey-native-store branch from 992d5bc to 8c85612 Compare December 15, 2024 19:00
Copy link
Member

@cofin cofin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think this is fine for the initial implementation. There is probably some larger restructuring we'll do on a few things for 3.0, and this may be part of it. However, i think it's better to get this implemented in smaller chunks.

@cofin
Copy link
Member

cofin commented Dec 15, 2024

@all-contributors add @ftsartek for docs, tests and code

Copy link
Contributor

@cofin

I've put up a pull request to add @ftsartek! 🎉

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3892

@cofin cofin merged commit 6c2bb39 into litestar-org:main Dec 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies This PR involves changes to the dependencies area/docs This PR involves changes to the documentation area/stores pr/external pr/internal size: medium Triage Required 🏥 This requires triage type/feat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants