Skip to content

Support TTI expiration in Redis Cache implementation #2649

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 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-redis</artifactId>
<version>3.2.0-SNAPSHOT</version>
<version>3.2.0-GH-2351-SNAPSHOT</version>

<name>Spring Data Redis</name>
<description>Spring Data module for Redis</description>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,14 @@ 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 ttl) {

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 = shouldExpireWithin(ttl)
? execute(name, connection -> connection.getEx(key, Expiration.from(ttl)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we fallback to get and expire if server doesn't support getex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback, @quaff.

I spoke with @mp911de about this earlier today and we decided this feature will only be included from Spring Data Redis 3.2 and later, which is based on the latest Jedis and Lettuce drivers supporting most of the features in the newest Redis server releases.

In addition, it is an opt-in feature. See here and here, defaulting to the previous behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

which is based on the latest Jedis and Lettuce drivers supporting most of the features in the newest Redis server releases.

I mean server not client doesn't support getex, it's a new command since redis 6.2.0, upgrading server is not convenient as client, many server still using 4.x and 5.x, especially service provided by private cloud.

Copy link
Member

@mp911de mp911de Jul 21, 2023

Choose a reason for hiding this comment

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

If the server does not support getex then you cannot use Time to Idle.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could fallback to get and expire.

Copy link
Contributor Author

@jxblum jxblum Jul 25, 2023

Choose a reason for hiding this comment

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

@quaff - I understood what you meant between client and server versions.

@christophstrobl and I are meeting this week on the topic of caching in SD Redis and using TTI-like expiration. Let me touch base with Christoph on this whether or not we will take into consideration Redis server version.

: execute(name, connection -> connection.get(key));

statistics.incGets(name);

Expand Down
17 changes: 14 additions & 3 deletions src/main/java/org/springframework/data/redis/cache/RedisCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.lang.reflect.Method;
import java.nio.ByteBuffer;
import java.time.Duration;
import java.util.Arrays;
import java.util.Collection;
import java.util.Map;
Expand Down Expand Up @@ -188,11 +189,21 @@ protected <T> T loadCacheValue(Object key, Callable<T> valueLoader) {
@Override
protected Object lookup(Object key) {

byte[] value = getCacheWriter().get(getName(), createAndConvertCacheKey(key));
byte[] value = getCacheConfiguration().isTtiExpirationEnabled()
? getCacheWriter().get(getName(), createAndConvertCacheKey(key), getTimeToLive(key))
: getCacheWriter().get(getName(), createAndConvertCacheKey(key));

return value != null ? deserializeCacheValue(value) : null;
}

private Duration getTimeToLive(Object key) {
return getTimeToLive(key, null);
}

private Duration getTimeToLive(Object key, @Nullable Object value) {
return getCacheConfiguration().getTtlFunction().getTimeToLive(key, value);
}

@Override
public void put(Object key, @Nullable Object value) {

Expand All @@ -208,7 +219,7 @@ public void put(Object key, @Nullable Object value) {
}

getCacheWriter().put(getName(), createAndConvertCacheKey(key), serializeCacheValue(cacheValue),
getCacheConfiguration().getTtlFunction().getTimeToLive(key, value));
getTimeToLive(key, value));
}

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

byte[] result = getCacheWriter().putIfAbsent(getName(), createAndConvertCacheKey(key),
serializeCacheValue(cacheValue), getCacheConfiguration().getTtlFunction().getTimeToLive(key, value));
serializeCacheValue(cacheValue), getTimeToLive(key, value));

return result != null ? new SimpleValueWrapper(fromStoreValue(deserializeCacheValue(result))) : null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@
public class RedisCacheConfiguration {

protected static final boolean DEFAULT_CACHE_NULL_VALUES = true;
protected static final boolean DEFAULT_ENABLE_TTI_EXPIRATION = false;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use TIME_TO_IDLE instead of TTI for more expressivity.

Copy link
Contributor Author

@jxblum jxblum Jul 25, 2023

Choose a reason for hiding this comment

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

Will do.

Ironically, I initially spelled it out in favor of being explicit, but ultimately opted to match our use of the "TTL" abbreviation. It is definitely easy to overlook and possibly confuse "TTI" with "TTL" on a brief glance.

protected static final boolean DEFAULT_USE_PREFIX = true;
protected static final boolean DO_NOT_CACHE_NULL_VALUES = false;
protected static final boolean DO_NOT_USE_PREFIX = false;
protected static final boolean ENABLE_IDLE_TIME_EXPIRATION = true;
Copy link
Member

Choose a reason for hiding this comment

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

@quaff has a good point regarding server versions. We cannot assume the application runs on the latest Redis version. If we'd enabled this for everyone, then all users with older Redis versions suddenly see failures on their target environments unless they reconfigure the cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did similar work before, like this:

	private volatile boolean getexSupported = true;

	@Override
	public Object getWithTti(String key, int timeToIdle, TimeUnit timeUnit) {
		BoundValueOperations operations = redisTemplate.boundValueOps(key);
		try {
			if (getexSupported)
				return timeToIdle > 0 ? operations.getAndExpire(timeToIdle, timeUnit) : operations.getAndPersist();
			// fallback to get and expire
			Object result = operations.get();
			if (result != null && timeToIdle > 0)
				operations.expire(timeToIdle, timeUnit);
			return result;
		} catch (RuntimeException e) {
			log.error(e.getMessage(), e);
			if (e.getCause() instanceof RedisCommandExecutionException) {
				getexSupported = false; // server.version < 6.2
				return getWithTti(key,  timeToIdle, timeUnit);
			}
			throw e;
		}
	}

Copy link
Contributor Author

@jxblum jxblum Jul 25, 2023

Choose a reason for hiding this comment

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

Please note, the ENABLE_IDLE_TIME_EXPIRATION constant was meant to express the preferred use of time-to-idle (TTI) expiration when "explicitly enabled" and is NOT a flag defaulting TTI to true. The "default" value for TTI is expressed constant DEFAULT_ENABLE_TTI_EXPIRATION, here.

I prefer to use "named" constants rather than "true", especially when calling constructors or methods accepting (multiple) boolean-typed arguments in order to make it clear "what" is actually being enabled. For instance.

This similar to the existing constants for not caching null values (disableCachingNullValues(); here) or not using a cache key prefix, (disableKeyPrefix(); here).

However, I realize the constant name (ENABLE_IDLE_TIME_EXPIRATION ) was a poor choice of words and I have changed this to USE_TIME_TO_IDLE_EXPIRATION. This is now, not unlike the constants for caching null values or using a cache key prefix.

Keep in mind, this constant (now USE_TIME_TO_IDLE_EXPIRATION) is only ever used when TTI is enabled by calling the enableTtiExpiration() method on the RedisCacheConfiguration instance, here. This makes it very explicit as to what is being set (TTI) and how (use (enable) TTI).

Copy link
Contributor Author

@jxblum jxblum Jul 25, 2023

Choose a reason for hiding this comment

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

@quaff - FYI, if @christophstrobl and I decide it makes sense to handle Redis server version for a (Redis) Cache.get(key), using either GETEX or a GET with EXPIRE (although, I am not certain SD Redis takes Redis server version into consideration anywhere else, though), then I would envision something a bit more robust, such as by (proactively) inspecting the Redis server version on construction of the DefaultRedisCacheWriter using the INFO command from RedisServerCommands given a RedisConnection (retrieved from the factory).


/**
* Default {@link RedisCacheConfiguration} using the following:
Expand Down Expand Up @@ -108,14 +110,18 @@ public static RedisCacheConfiguration defaultCacheConfig(@Nullable ClassLoader c

registerDefaultConverters(conversionService);

return new RedisCacheConfiguration(TtlFunction.persistent(), DEFAULT_CACHE_NULL_VALUES, DEFAULT_USE_PREFIX,
return new RedisCacheConfiguration(TtlFunction.persistent(),
DEFAULT_CACHE_NULL_VALUES,
DEFAULT_ENABLE_TTI_EXPIRATION,
DEFAULT_USE_PREFIX,
CacheKeyPrefix.simple(),
SerializationPair.fromSerializer(RedisSerializer.string()),
SerializationPair.fromSerializer(RedisSerializer.java(classLoader)),
conversionService);
}

private final boolean cacheNullValues;
private final boolean enableTtiExpiration;
Copy link
Member

Choose a reason for hiding this comment

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

Let's expand on the name to enableTimeToIdle instead of Tti.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I also renamed isTtiExpirationEnabled() to isTimeToIdleEnabled() and enableTtiExpiration() to enableTimeToIdle().

private final boolean usePrefix;

private final CacheKeyPrefix keyPrefix;
Expand All @@ -128,12 +134,13 @@ public static RedisCacheConfiguration defaultCacheConfig(@Nullable ClassLoader c
private final TtlFunction ttlFunction;

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

this.ttlFunction = ttlFunction;
this.cacheNullValues = cacheNullValues;
this.enableTtiExpiration = enableTtiExpiration;
this.usePrefix = usePrefix;
this.keyPrefix = keyPrefix;
this.keySerializationPair = keySerializationPair;
Expand Down Expand Up @@ -168,8 +175,9 @@ public RedisCacheConfiguration computePrefixWith(CacheKeyPrefix cacheKeyPrefix)

Assert.notNull(cacheKeyPrefix, "Function used to compute prefix must not be null");

return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), DEFAULT_USE_PREFIX,
cacheKeyPrefix, getKeySerializationPair(), getValueSerializationPair(), getConversionService());
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), isTtiExpirationEnabled(),
DEFAULT_USE_PREFIX, cacheKeyPrefix, getKeySerializationPair(), getValueSerializationPair(),
getConversionService());
}

/**
Expand All @@ -181,8 +189,9 @@ public RedisCacheConfiguration computePrefixWith(CacheKeyPrefix cacheKeyPrefix)
* @return new {@link RedisCacheConfiguration}.
*/
public RedisCacheConfiguration disableCachingNullValues() {
return new RedisCacheConfiguration(getTtlFunction(), DO_NOT_CACHE_NULL_VALUES, usePrefix(), getKeyPrefix(),
getKeySerializationPair(), getValueSerializationPair(), getConversionService());
return new RedisCacheConfiguration(getTtlFunction(), DO_NOT_CACHE_NULL_VALUES, isTtiExpirationEnabled(),
usePrefix(), getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(),
getConversionService());
}

/**
Expand All @@ -193,8 +202,30 @@ public RedisCacheConfiguration disableCachingNullValues() {
* @return new {@link RedisCacheConfiguration}.
*/
public RedisCacheConfiguration disableKeyPrefix() {
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), DO_NOT_USE_PREFIX,
getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(), getConversionService());
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), isTtiExpirationEnabled(),
DO_NOT_USE_PREFIX, getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(), getConversionService());
}

/**
* Enables {@literal time-to-idle (TTI) expiration} on {@link Cache} read operations,
* such as {@link Cache#get(Object)}.
* <p>
* Enabling this option applies the same {@link #getTtlFunction() TTL expiration policy} to {@link Cache} read
* operations as it does for {@link Cache} write operations. In effect, this will invoke the Redis {@literal GETEX}
* command in place of {@literal GET}.
* <p>
* Redis does not support the concept of {@literal TTI}, only {@literal TTL}. However, if {@literal TTL} expiration
* is applied to all {@link Cache} operations, both read and write alike, and {@link Cache} operations passed with
* expiration are used consistently across the application, then in effect, an application can achieve
* {@literal TTI} expiration-like behavior.
*
* @return this {@link RedisCacheConfiguration}.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: @since 3.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

* @see <a href="https://redis.io/commands/getex/">GETEX</a>
*/
public RedisCacheConfiguration enableTtiExpiration() {
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), ENABLE_IDLE_TIME_EXPIRATION,
usePrefix(), getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(),
getConversionService());
}

/**
Expand Down Expand Up @@ -222,8 +253,9 @@ public RedisCacheConfiguration entryTtl(TtlFunction ttlFunction) {

Assert.notNull(ttlFunction, "TtlFunction must not be null");

return new RedisCacheConfiguration(ttlFunction, getAllowCacheNullValues(), usePrefix(), getKeyPrefix(),
getKeySerializationPair(), getValueSerializationPair(), getConversionService());
return new RedisCacheConfiguration(ttlFunction, getAllowCacheNullValues(), isTtiExpirationEnabled(),
usePrefix(), getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(),
getConversionService());
}

/**
Expand All @@ -236,8 +268,8 @@ public RedisCacheConfiguration serializeKeysWith(SerializationPair<String> keySe

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

return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(),
keySerializationPair, getValueSerializationPair(), getConversionService());
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), isTtiExpirationEnabled(),
usePrefix(), getKeyPrefix(), keySerializationPair, getValueSerializationPair(), getConversionService());
}

/**
Expand All @@ -250,8 +282,8 @@ public RedisCacheConfiguration serializeValuesWith(SerializationPair<?> valueSer

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

return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(),
getKeySerializationPair(), valueSerializationPair, getConversionService());
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), isTtiExpirationEnabled(),
usePrefix(), getKeyPrefix(), getKeySerializationPair(), valueSerializationPair, getConversionService());
}

/**
Expand All @@ -264,8 +296,8 @@ public RedisCacheConfiguration withConversionService(ConversionService conversio

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

return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(),
getKeySerializationPair(), getValueSerializationPair(), conversionService);
return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), isTtiExpirationEnabled(),
usePrefix(), getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(), conversionService);
}

/**
Expand All @@ -275,6 +307,19 @@ public boolean getAllowCacheNullValues() {
return this.cacheNullValues;
}

/**
* Determines whether {@literal time-to-idle (TTI) expiration} has been enabled for caching.
* <p>
* Use {@link #enableTtiExpiration()} to opt-in and enable {@literal time-to-idle (TTI) expiration} for caching.
*
* @return {@literal true} if {@literal time-to-idle (TTI) expiration} was configured and enabled for caching.
* Defaults to {@literal false}.
* @see <a href="https://redis.io/commands/getex/">GETEX</a>
*/
public boolean isTtiExpirationEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: @since 3.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return this.enableTtiExpiration;
}

/**
* @return {@literal true} if cache keys need to be prefixed with the {@link #getKeyPrefixFor(String)} if present or
* the default which resolves to {@link Cache#getName()}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio
*/
static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectionFactory,
BatchStrategy batchStrategy) {
return lockingRedisCacheWriter(connectionFactory, Duration.ofMillis(50), TtlFunction.persistent(), batchStrategy);

return lockingRedisCacheWriter(connectionFactory, Duration.ofMillis(50), TtlFunction.persistent(),
batchStrategy);
}

/**
Expand All @@ -104,29 +106,43 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio

Assert.notNull(connectionFactory, "ConnectionFactory must not be null");

return new DefaultRedisCacheWriter(connectionFactory, sleepTime, lockTtlFunction, CacheStatisticsCollector.none(),
batchStrategy);
return new DefaultRedisCacheWriter(connectionFactory, sleepTime, lockTtlFunction,
CacheStatisticsCollector.none(), batchStrategy);
}

/**
* Write the given key/value pair to Redis and set the expiration time if defined.
* Get the binary value representation from Redis stored for the given key.
*
* @param name The cache name must not be {@literal null}.
* @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 name must not be {@literal null}.
* @param key must not be {@literal null}.
* @return {@literal null} if key does not exist.
* @see #get(String, byte[], Duration)
*/
void put(String name, byte[] key, byte[] value, @Nullable Duration ttl);
@Nullable
default byte[] get(String name, byte[] key) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's do the defaulting the other way round, that we introduce get(String name, byte[] key, @Nullable Duration ttl) as default method to not break existing implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

return get(name, key, null);
}

/**
* Get the binary value representation from Redis stored for the given key.
* Get the binary value representation from Redis stored for the given key and set the given
* {@link Duration TTL expiration} for the cache entry.
*
* @param name must not be {@literal null}.
* @param key must not be {@literal null}.
* @return {@literal null} if key does not exist.
* @param ttl {@link Duration} specifying the {@literal expiration timeout} for the cache entry.
* @return {@literal null} if key does not exist or has {@literal expired}.
*/
@Nullable
byte[] get(String name, byte[] key);
byte[] get(String name, byte[] key, @Nullable Duration ttl);

/**
* Write the given key/value pair to Redis and set the expiration time if defined.
*
* @param name The cache name must not be {@literal null}.
* @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}.
*/
void put(String name, byte[] key, byte[] value, @Nullable Duration ttl);

/**
* Write the given value to Redis if the key does not already exist.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.springframework.data.redis.core.types;

import java.time.Duration;
import java.util.Objects;
import java.util.concurrent.TimeUnit;

import org.springframework.lang.Nullable;
Expand Down Expand Up @@ -219,6 +220,25 @@ public boolean isUnixTimestamp() {
return false;
}

@Override
public boolean equals(Object obj) {

if (this == obj) {
return true;
}

if (!(obj instanceof Expiration that)) {
return false;
}

return this.getTimeUnit().toMillis(getExpirationTime()) == that.getTimeUnit().toMillis(that.getExpirationTime());
}

@Override
public int hashCode() {
return Objects.hash(getExpirationTime(), getTimeUnit());
Copy link
Member

Choose a reason for hiding this comment

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

We use Spring's ObjectUtils.nullSafeHashCode(…) to compute the hash code in other places.

Copy link
Contributor Author

@jxblum jxblum Jul 25, 2023

Choose a reason for hiding this comment

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

Spring Framework's ObjectUtils.nullSafeHashCode(..) method is a bit more verbose and sloppy, but no matter.

}

/**
* @author Christoph Strobl
* @since 2.4
Expand Down
Loading