Skip to content

Add support for GETEX in Spring Redis Cache #2643

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.

Sign up for GitHub

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

Add support for GETEX in Spring Redis Cache #2643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed43779
#GH-2351 - added maxIdle
Jul 16, 2023
2ad21ab
#GH-2351 - update maxIdle to entryMaxIdle
Jul 16, 2023
0f8d0e4
#GH-2351 - added tests
Jul 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
* @author Christoph Strobl
* @author Mark Paluch
* @author André Prata
* @author Shyngys Sapraliyev
* @since 2.0
*/
class DefaultRedisCacheWriter implements RedisCacheWriter {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jxblum jxblum Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Assert.notNull(name, "Name must not be null!");
Assert.notNull(key, "Key must not be null!");
Expand All @@ -104,7 +105,11 @@ public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl) {
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);
}
Comment on lines 105 to +112
Copy link
Contributor

@jxblum jxblum Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jxblum jxblum Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

return "OK";
Expand All @@ -114,12 +119,18 @@ public void put(String name, byte[] key, byte[] value, @Nullable Duration ttl) {
}

@Override
public byte[] get(String name, byte[] key) {
public byte[] get(String name, byte[] key, @Nullable Duration maxIdle) {

Assert.notNull(name, "Name must not be null!");
Assert.notNull(key, "Key must not be null!");

byte[] result = execute(name, connection -> connection.get(key));
byte[] result;

if (shouldExpireMaxIdleWithin(maxIdle)) {
result = execute(name, connection -> connection.getEx(key, Expiration.from(maxIdle)));
Copy link
Contributor

@jxblum jxblum Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The lesser of TTL and TTI should take precedence.

} else {
result = execute(name, connection -> connection.get(key));
}

statistics.incGets(name);

Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bytestreme bytestreme Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How about default method in RedisCacheWriter?

Copy link
Contributor

@jxblum jxblum Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Assert.notNull(name, "Name must not be null!");
Assert.notNull(key, "Key must not be null!");
Expand All @@ -152,7 +163,11 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat
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());
Comment on lines 163 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

The lesser of TTL and TTI should take precedence.

} else {
put = connection.setNX(key, value);
}
}

if (put) {
Expand Down Expand Up @@ -318,6 +333,10 @@ private static boolean shouldExpireWithin(@Nullable Duration ttl) {
return ttl != null && !ttl.isZero() && !ttl.isNegative();
}

private static boolean shouldExpireMaxIdleWithin(@Nullable Duration maxIdle) {
return maxIdle != null && !maxIdle.isZero() && !maxIdle.isNegative();
}

private static byte[] createCacheLockKey(String name) {
return (name + "~lock").getBytes(StandardCharsets.UTF_8);
}
Expand Down
8 changes: 5 additions & 3 deletions src/main/java/org/springframework/data/redis/cache/RedisCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
* @author Christoph Strobl
* @author Mark Paluch
* @author Piotr Mionskowski
* @author Shyngys Sapraliyev
* @see RedisCacheConfiguration
* @see RedisCacheWriter
* @since 2.0
Expand Down Expand Up @@ -82,7 +83,7 @@ protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfig
@Override
protected Object lookup(Object key) {

byte[] value = cacheWriter.get(name, createAndConvertCacheKey(key));
byte[] value = cacheWriter.get(name, createAndConvertCacheKey(key), cacheConfig.getMaxIdle());

if (value == null) {
return null;
Expand Down Expand Up @@ -145,7 +146,8 @@ public void put(Object key, @Nullable Object value) {
name));
}

cacheWriter.put(name, createAndConvertCacheKey(key), serializeCacheValue(cacheValue), cacheConfig.getTtl());
cacheWriter.put(name, createAndConvertCacheKey(key), serializeCacheValue(cacheValue),
cacheConfig.getTtl(), cacheConfig.getMaxIdle());
}

@Override
Expand All @@ -158,7 +160,7 @@ public ValueWrapper putIfAbsent(Object key, @Nullable Object value) {
}

byte[] result = cacheWriter.putIfAbsent(name, createAndConvertCacheKey(key), serializeCacheValue(cacheValue),
cacheConfig.getTtl());
cacheConfig.getTtl(), cacheConfig.getMaxIdle());

if (result == null) {
return null;
Expand Down
57 changes: 41 additions & 16 deletions src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@
*
* @author Christoph Strobl
* @author Mark Paluch
* @author Shyngys Sapraliyev
* @since 2.0
*/
public class RedisCacheConfiguration {

private final Duration ttl;
private final Duration maxIdle;
private final boolean cacheNullValues;
private final CacheKeyPrefix keyPrefix;
private final boolean usePrefix;
Expand All @@ -53,10 +55,11 @@ public class RedisCacheConfiguration {
private final ConversionService conversionService;

@SuppressWarnings("unchecked")
private RedisCacheConfiguration(Duration ttl, Boolean cacheNullValues, Boolean usePrefix, CacheKeyPrefix keyPrefix,
private RedisCacheConfiguration(Duration ttl, Duration maxIdle,
Boolean cacheNullValues, Boolean usePrefix, CacheKeyPrefix keyPrefix,
SerializationPair<String> keySerializationPair, SerializationPair<?> valueSerializationPair,
ConversionService conversionService) {

this.maxIdle = maxIdle;
this.ttl = ttl;
this.cacheNullValues = cacheNullValues;
this.usePrefix = usePrefix;
Expand Down Expand Up @@ -123,8 +126,8 @@ public static RedisCacheConfiguration defaultCacheConfig(@Nullable ClassLoader c

registerDefaultConverters(conversionService);

return new RedisCacheConfiguration(Duration.ZERO, true, true, CacheKeyPrefix.simple(),
SerializationPair.fromSerializer(RedisSerializer.string()),
return new RedisCacheConfiguration(Duration.ZERO, Duration.ZERO, true, true,
CacheKeyPrefix.simple(), SerializationPair.fromSerializer(RedisSerializer.string()),
SerializationPair.fromSerializer(RedisSerializer.java(classLoader)), conversionService);
}

Expand All @@ -138,8 +141,23 @@ public RedisCacheConfiguration entryTtl(Duration ttl) {

Assert.notNull(ttl, "TTL duration must not be null!");

return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, usePrefix, keyPrefix,
keySerializationPair, valueSerializationPair, conversionService);
}

/**
* Set the ttl to apply for cache entries on fetch
* Using with {@link Duration#ZERO} will not affect cache entries
*
* @param maxIdle must not be {@literal null}.
* @return new {@link RedisCacheConfiguration}.
*/
public RedisCacheConfiguration entryMaxIdle(Duration maxIdle) {

Assert.notNull(maxIdle, "maxIdle duration must not be null!");

return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, usePrefix, keyPrefix,
keySerializationPair, valueSerializationPair, conversionService);
}

/**
Expand Down Expand Up @@ -169,8 +187,8 @@ public RedisCacheConfiguration computePrefixWith(CacheKeyPrefix cacheKeyPrefix)

Assert.notNull(cacheKeyPrefix, "Function for computing prefix must not be null!");

return new RedisCacheConfiguration(ttl, cacheNullValues, true, cacheKeyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, true, cacheKeyPrefix,
keySerializationPair, valueSerializationPair, conversionService);
}

/**
Expand All @@ -182,8 +200,8 @@ public RedisCacheConfiguration computePrefixWith(CacheKeyPrefix cacheKeyPrefix)
* @return new {@link RedisCacheConfiguration}.
*/
public RedisCacheConfiguration disableCachingNullValues() {
return new RedisCacheConfiguration(ttl, false, usePrefix, keyPrefix, keySerializationPair, valueSerializationPair,
conversionService);
return new RedisCacheConfiguration(ttl, maxIdle, false, usePrefix, keyPrefix,
keySerializationPair, valueSerializationPair, conversionService);
}

/**
Expand All @@ -195,8 +213,8 @@ public RedisCacheConfiguration disableCachingNullValues() {
*/
public RedisCacheConfiguration disableKeyPrefix() {

return new RedisCacheConfiguration(ttl, cacheNullValues, false, keyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, false,
keyPrefix, keySerializationPair, valueSerializationPair, conversionService);
}

/**
Expand All @@ -209,7 +227,7 @@ public RedisCacheConfiguration withConversionService(ConversionService conversio

Assert.notNull(conversionService, "ConversionService must not be null!");

return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
}

Expand All @@ -223,7 +241,7 @@ public RedisCacheConfiguration serializeKeysWith(SerializationPair<String> keySe

Assert.notNull(keySerializationPair, "KeySerializationPair must not be null!");

return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
}

Expand All @@ -237,8 +255,8 @@ public RedisCacheConfiguration serializeValuesWith(SerializationPair<?> valueSer

Assert.notNull(valueSerializationPair, "ValueSerializationPair must not be null!");

return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair,
valueSerializationPair, conversionService);
return new RedisCacheConfiguration(ttl, maxIdle, cacheNullValues, usePrefix, keyPrefix,
keySerializationPair, valueSerializationPair, conversionService);
}

/**
Expand Down Expand Up @@ -297,6 +315,13 @@ public ConversionService getConversionService() {
return conversionService;
}

/**
* @return The max idle time for cache entries. Never {@literal null}.
*/
public Duration getMaxIdle() {
return maxIdle;
}

/**
* Add a {@link Converter} for extracting the {@link String} representation of a cache key if no suitable
* {@link Object#toString()} method is present.
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
*
* @author Christoph Strobl
* @author Mark Paluch
* @author Shyngys Sapraliyev
* @since 2.0
*/
public interface RedisCacheWriter extends CacheStatisticsProvider {
Expand Down Expand Up @@ -96,18 +97,20 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio
* @param key The key for the cache entry. Must not be {@literal null}.
* @param value The value stored for the key. Must not be {@literal null}.
* @param ttl Optional expiration time. Can be {@literal null}.
* @param maxIdle Optional max idle time. Can be {@literal null}.
*/
void put(String name, byte[] key, byte[] value, @Nullable Duration ttl);
void put(String name, byte[] key, byte[] value, @Nullable Duration ttl, @Nullable Duration maxIdle);

/**
* Get the binary value representation from Redis stored for the given key.
*
* @param name must not be {@literal null}.
* @param key must not be {@literal null}.
* @param maxIdle Optional max idle time. Can be {@literal null}.
* @return {@literal null} if key does not exist.
*/
@Nullable
byte[] get(String name, byte[] key);
byte[] get(String name, byte[] key, @Nullable Duration maxIdle);

/**
* Write the given value to Redis if the key does not already exist.
Expand All @@ -116,10 +119,11 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio
* @param key The key for the cache entry. Must not be {@literal null}.
* @param value The value stored for the key. Must not be {@literal null}.
* @param ttl Optional expiration time. Can be {@literal null}.
* @param maxIdle Optional max idle time. Can be {@literal null}.
* @return {@literal null} if the value has been written, the value stored for the key if it already exists.
*/
@Nullable
byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Duration ttl);
byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Duration ttl, @Nullable Duration maxIdle);

/**
* Remove the given key from Redis.
Expand Down
Loading