Skip to content

Add TTL support for CSC #4115

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

Closed
wants to merge 3 commits into from
Closed

Conversation

wp973
Copy link

@wp973 wp973 commented Mar 18, 2025

Add TTL support for CSC.

@wp973
Copy link
Author

wp973 commented Mar 18, 2025

Can someone help me take a look at this PR? :D

@wp973 wp973 changed the title feat(csc): add ttl support Add TTL support for CSC Mar 19, 2025
@ggivo
Copy link
Collaborator

ggivo commented Mar 19, 2025

Hi @wp973

Thank you for your contribution! The team is currently busy, so it may take some time before we can take a closer look at the suggested changes.

A brief review suggests that this PR could introduce significant performance considerations, especially for larger caches. Given that, I don’t think we should go down the path of implementing a full-blown cache within the project.

This feature request requires further discussion. Let's open an Issue where we can better understand the use cases and explore alternative approaches. One potential direction could be providing an example of an adapter using a third-party caching library instead of maintaining a custom implementation.

@wp973
Copy link
Author

wp973 commented Mar 20, 2025

Hi @wp973

Thank you for your contribution! The team is currently busy, so it may take some time before we can take a closer look at the suggested changes.

A brief review suggests that this PR could introduce significant performance considerations, especially for larger caches. Given that, I don’t think we should go down the path of implementing a full-blown cache within the project.

This feature request requires further discussion. Let's open an Issue where we can better understand the use cases and explore alternative approaches. One potential direction could be providing an example of an adapter using a third-party caching library instead of maintaining a custom implementation.

@ggivo This is very helpful for me, thanks! :D

Using popular third-party caching library(e.g, Caffeine) is a good choice, but it will also introduce third-party dependencies. Is this acceptable to Jedis?

@wp973 wp973 marked this pull request as draft March 20, 2025 08:55
@ggivo
Copy link
Collaborator

ggivo commented Mar 24, 2025

Using popular third-party caching library(e.g, Caffeine) is a good choice, but it will also introduce third-party dependencies. Is this acceptable to Jedis?

I don't think we should add a third-party dependency but just document/provide an example how to create an adapter of the Cache interface that can be used with Jedis using an arbitrary third-party library.

@ggivo
Copy link
Collaborator

ggivo commented Mar 24, 2025

Related to #4117

@ggivo
Copy link
Collaborator

ggivo commented Mar 24, 2025

@wp973
Is this PR still relevant or it is superseded by #4120?

@ggivo ggivo added the waiting-for-feedback We need additional information before we can continue label Mar 24, 2025
@wp973
Copy link
Author

wp973 commented Mar 27, 2025

Using popular third-party caching library(e.g, Caffeine) is a good choice, but it will also introduce third-party dependencies. Is this acceptable to Jedis?

I don't think we should add a third-party dependency but just document/provide an example how to create an adapter of the Cache interface that can be used with Jedis using an arbitrary third-party library.

Got it. Maybe I can try to provide an example. :D

Copy link

This pull request is marked stale. It will be closed in 30 days if it is not updated.

@github-actions github-actions bot added the stale label Apr 27, 2025
@github-actions github-actions bot closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale waiting-for-feedback We need additional information before we can continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants