Skip to content

Encapsulate Redis Scan CursorId #2802

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 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
38 changes: 38 additions & 0 deletions Jenkinsfile
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,26 @@ pipeline {
}
}
}
stage('Publish JDK 17 + Redis 7.2 Docker Image') {
when {
anyOf {
changeset "ci/openjdk17-redis-7.2/Dockerfile"
changeset "Makefile"
changeset "ci/pipeline.properties"
}
}
agent { label 'data' }
options { timeout(time: 20, unit: 'MINUTES') }

steps {
script {
def image = docker.build("springci/spring-data-with-redis-7.2:${p['java.main.tag']}", "--build-arg BASE=${p['docker.java.main.image']} --build-arg REDIS=${p['docker.redis.7.version']} -f ci/openjdk17-redis-7.2/Dockerfile .")
docker.withRegistry(p['docker.registry'], p['docker.credentials']) {
image.push()
}
}
}
}
stage('Publish JDK 21 + Redis 6.2 Docker Image') {
when {
anyOf {
Expand Down Expand Up @@ -134,6 +154,24 @@ pipeline {
}
}
}
stage("test: Redis 7") {
agent {
label 'data'
}
options { timeout(time: 30, unit: 'MINUTES') }
environment {
ARTIFACTORY = credentials("${p['artifactory.credentials']}")
DEVELOCITY_CACHE = credentials("${p['develocity.cache.credentials']}")
DEVELOCITY_ACCESS_KEY = credentials("${p['develocity.access-key']}")
}
steps {
script {
docker.image("harbor-repo.vmware.com/dockerhub-proxy-cache/springci/spring-data-with-redis-7.2:${p['java.main.tag']}").inside('-v $HOME:/tmp/jenkins-home') {
sh "PROFILE=none LONG_TESTS=true JENKINS_USER_NAME=${p['jenkins.user.name']} ci/test.sh"
}
}
}
Copy link
Member Author

@mp911de mp911de Jan 31, 2024

Choose a reason for hiding this comment

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

Something is off in indentation (tabs vs spaces).

Copy link
Member

@christophstrobl christophstrobl Feb 1, 2024

Choose a reason for hiding this comment

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

let me have a look at that.

}
}
}

Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

REDIS_VERSION:=6.2.6
REDIS_VERSION:=7.2.4
SPRING_PROFILE?=ci
SHELL=/bin/bash -euo pipefail

Expand Down Expand Up @@ -175,7 +175,7 @@ clobber:
work/redis/bin/redis-cli work/redis/bin/redis-server:
@mkdir -p work/redis

curl -sSL https://github.com/antirez/redis/archive/$(REDIS_VERSION).tar.gz | tar xzf - -C work
curl -sSL https://github.com/redis/redis/archive/$(REDIS_VERSION).tar.gz | tar xzf - -C work
$(MAKE) -C work/redis-$(REDIS_VERSION) -j
$(MAKE) -C work/redis-$(REDIS_VERSION) PREFIX=$(shell pwd)/work/redis install
rm -rf work/redis-$(REDIS_VERSION)
Expand Down
16 changes: 16 additions & 0 deletions ci/openjdk17-redis-7.2/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
ARG BASE
FROM ${BASE}
# Any ARG statements before FROM are cleared.
ARG REDIS

# Copy Spring Data Redis's Makefile into the container
COPY ./Makefile /

RUN set -eux; \
# sed -i -e 's/http/https/g' /etc/apt/sources.list ; \
apt-get update ; \
apt-get install -y build-essential ; \
make work/redis/bin/redis-cli work/redis/bin/redis-server REDIS_VERSION=${REDIS}; \
chmod -R o+rw work; \
apt-get clean; \
rm -rf /var/lib/apt/lists/*;
1 change: 1 addition & 0 deletions ci/pipeline.properties
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ docker.mongodb.7.0.version=7.0.2

# Supported versions of Redis
docker.redis.6.version=6.2.13
docker.redis.7.version=7.2.4
Copy link
Member Author

Choose a reason for hiding this comment

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

Mind updating the property in Spring Data Build as well?

Copy link
Member

Choose a reason for hiding this comment

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

created the task already spring-projects/spring-data-build#2212 waiting for final review.


# Supported versions of Cassandra
docker.cassandra.3.version=3.11.16
Expand Down
2 changes: 1 addition & 1 deletion ci/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export JENKINS_USER=${JENKINS_USER_NAME}
export GRADLE_ENTERPRISE_ACCESS_KEY=${DEVELOCITY_ACCESS_KEY}

# Execute maven test
MAVEN_OPTS="-Duser.name=${JENKINS_USER} -Duser.home=/tmp/jenkins-home" ./mvnw -s settings.xml clean test -P${PROFILE} -DrunLongTests=${LONG_TESTS:-false} -U -B
MAVEN_OPTS="-Duser.name=${JENKINS_USER} -Duser.home=/tmp/jenkins-home" ./mvnw -s settings.xml clean test -P${PROFILE} -DrunLongTests=${LONG_TESTS:-false} -Dgradle.cache.local.enabled=false -Dgradle.cache.remote.enabled=false -U -B
Copy link
Member Author

Choose a reason for hiding this comment

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

@erichaagdev Can you provide guidance on how to not skip tests when running builds against different build stages where the only change is the Redis or JDK version?

Choose a reason for hiding this comment

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

This PR will cause all tests to re-run in builds with these Maven options since it disables the build cache.

Test results come from cache when all inputs to the surefire/failsafe goal match a previous invocation in the cache. These inputs include the JDK version and the runtime classpath. So, assuming you have Redis in the classpath, these tests should re-execute on these sorts of changes even with the cache enabled.

Have you seen different behavior than this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR will cause all tests to re-run in builds with these Maven options since it disables the build cache.

That is intended because we want to run integration tests against a different Redis version. Our Redis service is an externally managed service that does not affect the classpath. We would need to have to tell the caching inputs about a changed cache key somehow.

Copy link

@clayburn clayburn Feb 2, 2024

Choose a reason for hiding this comment

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

I see. Essentially, you want to represent the version of Redis as an input to the surefire/failsafe goals. The documentation for that can be found here.

More specifically, you could either:

  • Pass in the Redis version as a system property to <argLine>
  • Specify some file in the Redis installation that represents the version as an input file

Either of these would invalidate the cache when the Redis version changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the detail. <argLine> seems the most promising approach for now.


# Capture resulting exit code from maven (pass/fail)
RESULT=$?
Expand Down
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.3.0-SNAPSHOT</version>
<version>3.3.0-GH-2796-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 @@ -275,14 +275,14 @@ public Cursor<Entry<byte[], byte[]>> hScan(byte[] key, ScanOptions options) {
return new ScanCursor<Entry<byte[], byte[]>>(options) {

@Override
protected ScanIteration<Entry<byte[], byte[]>> doScan(long cursorId, ScanOptions options) {
protected ScanIteration<Entry<byte[], byte[]>> doScan(CursorId cursorId, ScanOptions options) {

ScanParams params = JedisConverters.toScanParams(options);

ScanResult<Entry<byte[], byte[]>> result = connection.getCluster().hscan(key,
JedisConverters.toBytes(Long.toUnsignedString(cursorId)),
JedisConverters.toBytes(cursorId),
params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()), result.getResult());
return new ScanIteration<>(CursorId.of(result.getCursor()), result.getResult());
}
}.open();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,11 @@ Cursor<byte[]> scan(RedisClusterNode node, ScanOptions options) {
return new ScanCursor<byte[]>(0, options) {

@Override
protected ScanIteration<byte[]> doScan(long cursorId, ScanOptions options) {
protected ScanIteration<byte[]> doScan(CursorId cursorId, ScanOptions options) {

ScanParams params = JedisConverters.toScanParams(options);
ScanResult<String> result = client.scan(Long.toUnsignedString(cursorId), params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()),
ScanResult<String> result = client.scan(cursorId.getCursorId(), params);
return new ScanIteration<>(CursorId.of(result.getCursor()),
JedisConverters.stringListToByteList().convert(result.getResult()));
}
}.open();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,12 +394,11 @@ public Cursor<byte[]> sScan(byte[] key, ScanOptions options) {
return new ScanCursor<byte[]>(options) {

@Override
protected ScanIteration<byte[]> doScan(long cursorId, ScanOptions options) {
protected ScanIteration<byte[]> doScan(CursorId cursorId, ScanOptions options) {

ScanParams params = JedisConverters.toScanParams(options);
ScanResult<byte[]> result = connection.getCluster().sscan(key,
JedisConverters.toBytes(Long.toUnsignedString(cursorId)), params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()), result.getResult());
ScanResult<byte[]> result = connection.getCluster().sscan(key, JedisConverters.toBytes(cursorId), params);
return new ScanIteration<>(CursorId.of(result.getCursor()), result.getResult());
}
}.open();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1079,13 +1079,13 @@ public Cursor<Tuple> zScan(byte[] key, ScanOptions options) {
return new ScanCursor<Tuple>(options) {

@Override
protected ScanIteration<Tuple> doScan(long cursorId, ScanOptions options) {
protected ScanIteration<Tuple> doScan(CursorId cursorId, ScanOptions options) {

ScanParams params = JedisConverters.toScanParams(options);

ScanResult<redis.clients.jedis.resps.Tuple> result = connection.getCluster().zscan(key,
JedisConverters.toBytes(Long.toUnsignedString(cursorId)), params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()),
JedisConverters.toBytes(cursorId), params);
return new ScanIteration<>(CursorId.of(result.getCursor()),
JedisConverters.tuplesToTuples().convert(result.getResult()));
}
}.open();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import org.springframework.data.redis.connection.convert.StringToRedisClientInfoConverter;
import org.springframework.data.redis.connection.zset.DefaultTuple;
import org.springframework.data.redis.connection.zset.Tuple;
import org.springframework.data.redis.core.Cursor;
import org.springframework.data.redis.core.ScanOptions;
import org.springframework.data.redis.core.types.Expiration;
import org.springframework.data.redis.core.types.RedisClientInfo;
Expand Down Expand Up @@ -175,6 +176,17 @@ public static byte[] toBytes(Number source) {
return toBytes(String.valueOf(source));
}

/**
* Convert the given {@link org.springframework.data.redis.core.Cursor.CursorId} into its binary representation.
*
* @param source must not be {@literal null}.
* @return the binary representation.
* @since 3.3
*/
static byte[] toBytes(Cursor.CursorId source) {
return toBytes(source.getCursorId());
}

@Nullable
public static byte[] toBytes(@Nullable String source) {
return source == null ? null : SafeEncoder.encode(source);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.springframework.data.redis.connection.RedisHashCommands;
import org.springframework.data.redis.connection.convert.Converters;
import org.springframework.data.redis.core.Cursor;
import org.springframework.data.redis.core.Cursor.CursorId;
import org.springframework.data.redis.core.KeyBoundCursor;
import org.springframework.data.redis.core.ScanIteration;
import org.springframework.data.redis.core.ScanOptions;
Expand Down Expand Up @@ -149,8 +150,7 @@ public List<Entry<byte[], byte[]>> hRandFieldWithValues(byte[] key, long count)

List<Entry<byte[], byte[]>> convertedMapEntryList = new ArrayList<>(mapEntryList.size());

mapEntryList.forEach(entry ->
convertedMapEntryList.add(Converters.entryOf(entry.getKey(), entry.getValue())));
mapEntryList.forEach(entry -> convertedMapEntryList.add(Converters.entryOf(entry.getKey(), entry.getValue())));

return convertedMapEntryList;

Expand Down Expand Up @@ -219,24 +219,17 @@ public List<byte[]> hVals(byte[] key) {

@Override
public Cursor<Entry<byte[], byte[]>> hScan(byte[] key, ScanOptions options) {
return hScan(key, 0, options);
return hScan(key, CursorId.initial(), options);
}

/**
* @since 1.4
* @param key
* @param cursorId
* @param options
* @return
*/
public Cursor<Entry<byte[], byte[]>> hScan(byte[] key, long cursorId, ScanOptions options) {
public Cursor<Entry<byte[], byte[]>> hScan(byte[] key, CursorId cursorId, ScanOptions options) {

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

return new KeyBoundCursor<Entry<byte[], byte[]>>(key, cursorId, options) {

@Override
protected ScanIteration<Entry<byte[], byte[]>> doScan(byte[] key, long cursorId, ScanOptions options) {
protected ScanIteration<Entry<byte[], byte[]>> doScan(byte[] key, CursorId cursorId, ScanOptions options) {

if (isQueueing() || isPipelined()) {
throw new InvalidDataAccessApiUsageException("'HSCAN' cannot be called in pipeline / transaction mode");
Expand All @@ -245,9 +238,8 @@ protected ScanIteration<Entry<byte[], byte[]>> doScan(byte[] key, long cursorId,
ScanParams params = JedisConverters.toScanParams(options);

ScanResult<Entry<byte[], byte[]>> result = connection.getJedis().hscan(key,
JedisConverters.toBytes(Long.toUnsignedString(cursorId)),
params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()), result.getResult());
JedisConverters.toBytes(cursorId), params);
return new ScanIteration<>(CursorId.of(result.getCursor()), result.getResult());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.springframework.data.redis.connection.ValueEncoding.RedisValueEncoding;
import org.springframework.data.redis.connection.convert.Converters;
import org.springframework.data.redis.core.Cursor;
import org.springframework.data.redis.core.Cursor.CursorId;
import org.springframework.data.redis.core.KeyScanOptions;
import org.springframework.data.redis.core.ScanCursor;
import org.springframework.data.redis.core.ScanIteration;
Expand Down Expand Up @@ -131,7 +132,7 @@ public Set<byte[]> keys(byte[] pattern) {

@Override
public Cursor<byte[]> scan(ScanOptions options) {
return scan(0, options != null ? options : ScanOptions.NONE);
return scan(CursorId.initial(), options != null ? options : ScanOptions.NONE);
}

/**
Expand All @@ -140,12 +141,12 @@ public Cursor<byte[]> scan(ScanOptions options) {
* @param options
* @return
*/
public Cursor<byte[]> scan(long cursorId, ScanOptions options) {
public Cursor<byte[]> scan(CursorId cursorId, ScanOptions options) {

return new ScanCursor<byte[]>(cursorId, options) {

@Override
protected ScanIteration<byte[]> doScan(long cursorId, ScanOptions options) {
protected ScanIteration<byte[]> doScan(CursorId cursorId, ScanOptions options) {

if (isQueueing() || isPipelined()) {
throw new InvalidDataAccessApiUsageException("'SCAN' cannot be called in pipeline / transaction mode");
Expand All @@ -165,12 +166,12 @@ protected ScanIteration<byte[]> doScan(long cursorId, ScanOptions options) {
}

if (type != null) {
result = connection.getJedis().scan(JedisConverters.toBytes(Long.toUnsignedString(cursorId)), params, type);
result = connection.getJedis().scan(JedisConverters.toBytes(cursorId), params, type);
} else {
result = connection.getJedis().scan(JedisConverters.toBytes(Long.toUnsignedString(cursorId)), params);
result = connection.getJedis().scan(JedisConverters.toBytes(cursorId), params);
}

return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()), result.getResult());
return new ScanIteration<>(CursorId.of(result.getCursor()), result.getResult());
}

protected void doClose() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.springframework.dao.InvalidDataAccessApiUsageException;
import org.springframework.data.redis.connection.RedisSetCommands;
import org.springframework.data.redis.core.Cursor;
import org.springframework.data.redis.core.Cursor.CursorId;
import org.springframework.data.redis.core.KeyBoundCursor;
import org.springframework.data.redis.core.ScanIteration;
import org.springframework.data.redis.core.ScanOptions;
Expand Down Expand Up @@ -206,34 +207,33 @@ public Long sUnionStore(byte[] destKey, byte[]... keys) {

@Override
public Cursor<byte[]> sScan(byte[] key, ScanOptions options) {
return sScan(key, 0, options);
return sScan(key, CursorId.initial(), options);
}

/**
* @since 1.4
* @param key
* @param cursorId
* @param options
* @return
* @since 3.2.1
*/
public Cursor<byte[]> sScan(byte[] key, long cursorId, ScanOptions options) {
public Cursor<byte[]> sScan(byte[] key, CursorId cursorId, ScanOptions options) {

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

return new KeyBoundCursor<byte[]>(key, cursorId, options) {

@Override
protected ScanIteration<byte[]> doScan(byte[] key, long cursorId, ScanOptions options) {
protected ScanIteration<byte[]> doScan(byte[] key, CursorId cursorId, ScanOptions options) {

if (isQueueing() || isPipelined()) {
throw new InvalidDataAccessApiUsageException("'SSCAN' cannot be called in pipeline / transaction mode");
}

ScanParams params = JedisConverters.toScanParams(options);

ScanResult<byte[]> result = connection.getJedis().sscan(key,
JedisConverters.toBytes(Long.toUnsignedString(cursorId)), params);
return new ScanIteration<>(Long.parseUnsignedLong(result.getCursor()), result.getResult());
ScanResult<byte[]> result = connection.getJedis().sscan(key, JedisConverters.toBytes(cursorId), params);
return new ScanIteration<>(CursorId.of(result.getCursor()), result.getResult());
}

protected void doClose() {
Expand Down
Loading