Add support for GETEX in Spring Redis Cache #2643

bytestreme · 2023-07-16T21:00:58Z

You have read the Spring Data contribution guidelines.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

bytestreme · 2023-07-16T21:03:08Z

resolves #2351

bytestreme · 2023-07-16T21:04:37Z

mp911de · 2023-07-17T14:30:32Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -133,7 +144,7 @@ public byte[] get(String name, byte[] key) {
 	}

 	@Override
-	public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Duration ttl) {
+	public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Duration ttl, @Nullable Duration maxIdle) {


This change doesn't make sense because there can be only one TTL and instead of changing public API, we rather should apply maxIdle defaulting on the calling side.

How about default method in RedisCacheWriter?

I don't think this signature change makes any sense in any of the changed RedisCacheWriter methods: get(..), put(..) or putIfAbsent(..); see below.

I'd prefer, if we decide to add any option to configure TTI (overriding TTL when TTI < TTL on gets) per cache entry, by key, at all, then the configuration would be included in the RedisCacheConfiguration per cache (not unlike TTL) and the RedisCacheConfiguration object would be passed to the components of the cache implementation to alter the caching behavior as needed. Otherwise, each new option will only increase the signature footprint, which is not desirable.

mp911de · 2023-07-17T14:30:50Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -93,7 +94,7 @@ class DefaultRedisCacheWriter implements RedisCacheWriter {
 	}

 	@Override
-	public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl) {
+	public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl, @Nullable Duration maxIdle) {


This change doesn't make sense because there can be only one TTL and instead of changing public API, we rather should apply maxIdle defaulting on the calling side.

Additionally, "maxIdle" is a misnomer in this case and does not accurately reflect the behavior of "idle" data access in general and idle-timeout expiration (TTL) policies in particular. See my comment in Issue #2351.

mp911de · 2023-07-18T08:59:30Z

@jxblum can you have a look as well? Introducing an expire after idle (Setting the TTL on GET) makes sense as we have already support for GETEX. I just wonder about the configuration side, whether we want to enable our TTL function for idle or whether we want to introduce another function to determine the Idle time for a key when getting the value. WDYT?

jxblum · 2023-07-19T23:53:04Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

 			if (shouldExpireWithin(ttl)) {
 				connection.set(key, value, Expiration.from(ttl.toMillis(), TimeUnit.MILLISECONDS), SetOption.upsert());
 			} else {
-				connection.set(key, value);
+				if (shouldExpireMaxIdleWithin(maxIdle)) {
+					connection.set(key, value, Expiration.from(maxIdle), SetOption.upsert());
+				} else {
+					connection.set(key, value);
+				}


As @mp911de alluded to above, there is technically only 1 expiration policy in Redis for a given key: TTL.

Expiration (and specifically, TTL) is something that is already handled in Spring Data Redis as of 2.7.x (and beyond: 3.0.x, 3.1.x, and currently, 3.2.x), which is apparent in lines 105-107 shown above.

Plus, what happens when TTL is less than TTI? The lesser of TTL and TTI should take precedence. (???)

Also note, as of Spring Data Redis 3.2.x (currently in development; latest version is 3.2.0-M1) TTL expiration can now be computed dynamically at runtime, per cache entry by key and value using the new TtlFunction interface.

Technically, TTI expiration policies (e.g. timeouts) have no bearing on cache entry creates or updates (upserts). TTL expiration is only updated when another create or update occurs after the initial create. Read access (e.g. get(key)) does not change TTL.

This would apply to RedisCacheWriter.putIfAbsent(..) as well.

jxblum · 2023-07-19T23:54:35Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

+		byte[] result;
+
+		if (shouldExpireMaxIdleWithin(maxIdle)) {
+			result = execute(name, connection -> connection.getEx(key, Expiration.from(maxIdle)));


The lesser of TTL and TTI should take precedence.

jxblum · 2023-07-20T00:01:30Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

 				if (shouldExpireWithin(ttl)) {
 					put = connection.set(key, value, Expiration.from(ttl), SetOption.ifAbsent());
 				} else {
-					put = connection.setNX(key, value);
+					if (shouldExpireMaxIdleWithin(maxIdle)) {
+						put = connection.set(key, value, Expiration.from(maxIdle), SetOption.ifAbsent());


The lesser of TTL and TTI should take precedence.

jxblum · 2023-07-20T00:04:35Z

@bytestreme & @mp911de -

First, please see my comments in Issue #2351, here, specifically here, and in conclusion, here.

jxblum · 2023-07-20T00:41:23Z

@mp911de - I agree. Adding an expiration on Cache.get(key) in the context of (Spring Data) Redis's cache implementation makes sense.

However, I would caution that Redis really does not technically have a proper notion of cache entry, idle-timeout expiration (TTI). It is essentially only TTL, or effectively TTI when GETEX along with SET including expiration options are used "consistently" across the application system architecture, and specifically, when caching.

It seems to me, frameworks like Redisson offer TTI as an additional feature implemented and supported by the framework itself outside of Redis. Without the use of OBJECT IDLETIME and an external expiration monitor, there is really no TTI, and juggling TTL with TTI could get messy and confusing for users.

As such, I think we should keep with our current TtlFunction interface contract, rather than adding an additional setting for TTI, and modify RedisCache.lookup(key) along with (Default)RedisCacheWriter.get(cacheName, key) to be, something like:

class RedisCache {

    protected Object lookup(Object key) {

        byte[] value = getCacheWriter().get(getName(), createAndConvertCacheKey(key), 
            getTtlFunction().getTimeToLive(key, null)); 

         // return deserialized, non-null value or null
    }
}

And:

class DefaultRedisCacheWriter {

    public byte[] get(String name, byte[] key, Duration ttl) {

        // ...

        byte[] result = ttl != null 
            ? execute(name, connection -> connection.getEx(key, toExpiration(ttl)))
            : execute(name, connection -> connection.get(key));
    }

We now support time-to-idle (TTI) expiration policies in Spring Data Redis's Cache implementation for cache reads. This TTI behavior is achieved with the use of the Redis GETEX command an consistently using the same TTL configuration for all cache operations when TTI is enabled. Closes spring-projects#2351 Original pull request: spring-projects#2643

We now support time-to-idle (TTI) expiration policies for cache reads. The TTI implementation is achieved with the use of the Redis GETEX command on Cache.get(key) operations as well as consistently using the same TTL configuration for all cache operations when TTI is enabled and TTL expiration has been configured, with the use of a TtlFunction or fixed Duration. Closes spring-projects#2351 Original pull request: spring-projects#2643

jxblum · 2023-07-21T01:37:11Z

See PR #2649.

We now support time-to-idle (TTI) expiration policies for cache reads. The TTI implementation is achieved with the use of the Redis GETEX command on Cache.get(key) operations as well as consistently using the same TTL configuration for all cache operations when TTI is enabled and TTL expiration has been configured, with the use of a TtlFunction or fixed Duration. Closes #2351 Original pull request: #2643

Order imports to match code style. Use one line per sentence in documentation. Original pull request: #2643

Shyngys Sapraliyev added 3 commits July 17, 2023 01:13

#spring-projectsGH-2351 - added maxIdle

ed43779

#spring-projectsGH-2351 - update maxIdle to entryMaxIdle

2ad21ab

#spring-projectsGH-2351 - added tests

0f8d0e4

bytestreme changed the title ~~Gh 2351~~ #GH-2351 Jul 16, 2023

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 16, 2023

mp911de changed the title ~~#GH-2351~~ Adding support to GETEX in Spring Redis Cache Jul 17, 2023

mp911de self-assigned this Jul 17, 2023

mp911de requested changes Jul 17, 2023

View reviewed changes

mp911de assigned jxblum Jul 18, 2023

mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 18, 2023

jxblum mentioned this pull request Jul 19, 2023

Add support for GETEX in Spring Redis Cache #2351

Closed

jxblum reviewed Jul 19, 2023

View reviewed changes

jxblum reviewed Jul 20, 2023

View reviewed changes

jxblum mentioned this pull request Jul 21, 2023

Support TTI expiration in Redis Cache implementation #2649

Closed

jxblum changed the title ~~Adding support to GETEX in Spring Redis Cache~~ Add support for GETEX in Spring Redis Cache Jul 21, 2023

mp911de added status: superseded An issue that has been superseded by another and removed type: enhancement A general enhancement labels Jul 21, 2023

mp911de closed this Jul 21, 2023

christophstrobl added a commit that referenced this pull request Aug 1, 2023

Polishing

5568ffd

Order imports to match code style. Use one line per sentence in documentation. Original pull request: #2643

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GETEX in Spring Redis Cache #2643

Add support for GETEX in Spring Redis Cache #2643

bytestreme commented Jul 16, 2023 •

edited

Loading

bytestreme commented Jul 16, 2023

bytestreme commented Jul 16, 2023

mp911de Jul 17, 2023

bytestreme Jul 17, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

mp911de Jul 17, 2023

jxblum Jul 19, 2023 •

edited

Loading

mp911de commented Jul 18, 2023

jxblum Jul 19, 2023 •

edited

Loading

jxblum Jul 20, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

jxblum Jul 20, 2023

jxblum commented Jul 20, 2023

jxblum commented Jul 20, 2023 •

edited

Loading

jxblum commented Jul 21, 2023

Add support for GETEX in Spring Redis Cache #2643

Add support for GETEX in Spring Redis Cache #2643

Conversation

bytestreme commented Jul 16, 2023 • edited Loading

bytestreme commented Jul 16, 2023

bytestreme commented Jul 16, 2023

mp911de Jul 17, 2023

Choose a reason for hiding this comment

bytestreme Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

mp911de Jul 17, 2023

Choose a reason for hiding this comment

jxblum Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

mp911de commented Jul 18, 2023

jxblum Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Jul 20, 2023

Choose a reason for hiding this comment

jxblum commented Jul 20, 2023

jxblum commented Jul 20, 2023 • edited Loading

jxblum commented Jul 21, 2023

bytestreme commented Jul 16, 2023 •

edited

Loading

bytestreme Jul 17, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

jxblum Jul 20, 2023 •

edited

Loading

jxblum Jul 19, 2023 •

edited

Loading

jxblum commented Jul 20, 2023 •

edited

Loading