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

Add Kredis#redis_version #149

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

heka1024
Copy link
Contributor

@heka1024 heka1024 commented Mar 8, 2024

There are many commands supported on only high versions of redis. For example LPOS or NX option in EXPIRE. So, if we can get version of current redis, we can implement some commands more effectively. For example, we can effectively add include? method for List(#148) using LPOS. Alse we can make #143 more effectively using NX option on EXPIRE.

Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Too costly to use for runtime feature detection. Need a different approach if you'd like to use newer Redis capabilities.

Simplest approach, really, is to bump the minimum Redis version to something more modern. Redis 5 is ancient now.

lib/kredis/info.rb Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Daer <jeremydaer@gmail.com>
@heka1024
Copy link
Contributor Author

@jeremy
At first, I looked at rails' implementation and thought it wouldn't be a huge burnden. (https://github.com/rails/rails/blob/4cfdcfef8eedb28a4e0f5d12fae80c2aade02a4c/activesupport/lib/active_support/cache/redis_cache_store.rb#L450-L461) But as you said, it might cause some performance issue. Then, how about memoize result of INFO or define_method dynamically? What I thought is like https://github.com/rails/rails/blob/e5124aed3fdcacb05204391e236c4fd60a1e6e74/activesupport/lib/active_support/notifications/instrumenter.rb#L218-L226.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants