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

Allow Setting Redis Key Experation #1666

Closed
Xuanwo opened this issue Mar 18, 2023 Discussed in #1665 · 8 comments · Fixed by #2027
Closed

Allow Setting Redis Key Experation #1666

Xuanwo opened this issue Mar 18, 2023 Discussed in #1665 · 8 comments · Fixed by #2027

Comments

@Xuanwo
Copy link
Collaborator

Xuanwo commented Mar 18, 2023

Discussed in #1665

Originally posted by jretterer-prol March 18, 2023
In using sccache with redis along with the allkeys-lru eviction policy I was seeing keys being removed from redis even thought the allocated memory space was not fully utilized. That led me to check the TTL on the redis keys and found one is set for sccache keys.

127.0.0.1:6379> TTL c/7/8/c78004b4adb30628728df15ce1e99e9f862814aaa9473c1582b5b996e823439c
(integer) 86258

After finding that I tried to find a way to adjust this but I can't seam to find it. So, is there a way to adjust this expiry time for redis keys or even better, set it so the time is refreshed when keys are read?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Mar 18, 2023

Note: OpenDAL already supports setting a TTL, so we only need to add the configuration value on the sccache side.

@Kinrany
Copy link

Kinrany commented Apr 28, 2023

Is there an option for turning off key expiration completely in the meantime? With LRU unused keys will naturally be evicted over time.

The Redis doc would probably be a good place for this too.

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Apr 28, 2023

Is there an option for turning off key expiration completely in the meantime?

Yes, I think we can skip the TTL setting if it's 0. Would you like to give it a try?

@Kinrany
Copy link

Kinrany commented Apr 28, 2023

Yes please! How do I do that?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Apr 28, 2023

Yes please! How do I do that?

By the way, we are not setting any TTL on redis now.

@Kinrany
Copy link

Kinrany commented Apr 28, 2023

Hmm. Brief search suggests that Redis doesn't have a default TTL either. So who is setting it? A different default in OpenDAL?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Apr 28, 2023

A different default in OpenDAL?

No, opendal doesn't set TTL by default.

@Kinrany
Copy link

Kinrany commented Apr 28, 2023

Would setting TTL accomplish anything with a default Redis instance then? I'm trying to understand if that would be solving the right problem. Where does the current value come from? Do we know?

sylvestre pushed a commit that referenced this issue Jan 12, 2024
* feat(redis): add support for ttl

* chore(redis): rustfmt

* chore(redis): fix doc

* chore(redis): fix keys to cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants