From e67d71bf873f7efec542102884d95902a891c178 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 19 May 2016 09:00:26 +0200 Subject: [PATCH] DATAREDIS-513 - Fix RedisServerCommands.time() failure when in pipeline mode. We fixed a glitch in Jedis/Lettuce RedisConneciton TIME command when used in pipeline or transaction mode. Original pull request: #199. --- .../connection/jedis/JedisConnection.java | 30 +++++++++------ .../connection/jedis/JedisConverters.java | 34 +++++++++++++---- .../connection/lettuce/LettuceConnection.java | 38 ++++++++++--------- .../connection/lettuce/LettuceConverters.java | 21 +++++++++- .../AbstractConnectionIntegrationTests.java | 15 +++++--- 5 files changed, 93 insertions(+), 45 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index 94ffd486a..97e56001a 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -1,12 +1,12 @@ /* * Copyright 2011-2016 the original author or authors. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -47,7 +47,7 @@ import org.springframework.data.redis.connection.RedisZSetCommands; import org.springframework.data.redis.connection.ReturnType; import org.springframework.data.redis.connection.SortParameters; import org.springframework.data.redis.connection.Subscription; -import org.springframework.data.redis.connection.convert.Converters; +import org.springframework.data.redis.connection.convert.ListConverter; import org.springframework.data.redis.connection.convert.TransactionResultConverter; import org.springframework.data.redis.core.Cursor; import org.springframework.data.redis.core.KeyBoundCursor; @@ -788,8 +788,8 @@ public class JedisConnection extends AbstractRedisConnection { /* * @see DATAREDIS-286 to avoid overflow in Jedis - * - * TODO Remove this workaround when we upgrade to a Jedis version that contains a + * + * TODO Remove this workaround when we upgrade to a Jedis version that contains a * fix for: https://github.com/xetorthio/jedis/pull/575 */ if (seconds > Integer.MAX_VALUE) { @@ -3181,13 +3181,21 @@ public class JedisConnection extends AbstractRedisConnection { @Override public Long time() { - List serverTimeInformation = this.jedis.time(); + try { - Assert.notEmpty(serverTimeInformation, "Received invalid result from server. Expected 2 items in collection."); - Assert.isTrue(serverTimeInformation.size() == 2, - "Received invalid nr of arguments from redis server. Expected 2 received " + serverTimeInformation.size()); + if (isPipelined()) { + pipeline(new JedisResult(pipeline.time(), JedisConverters.toTimeConverter())); + return null; + } - return Converters.toTimeMillis(serverTimeInformation.get(0), serverTimeInformation.get(1)); + if (isQueueing()) { + transaction(new JedisResult(transaction.time(), JedisConverters.toTimeConverter())); + return null; + } + return JedisConverters.toTimeConverter().convert(jedis.time()); + } catch (Exception ex) { + throw convertJedisAccessException(ex); + } } /* diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java index c241c60a6..cdd5efe9b 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java @@ -58,7 +58,7 @@ import redis.clients.util.SafeEncoder; /** * Jedis type converters. - * + * * @author Jennifer Hickey * @author Christoph Strobl * @author Thomas Darimont @@ -79,6 +79,7 @@ abstract public class JedisConverters extends Converters { private static final Converter OBJECT_TO_CLUSTER_NODE_CONVERTER; private static final Converter EXPIRATION_TO_COMMAND_OPTION_CONVERTER; private static final Converter SET_OPTION_TO_COMMAND_OPTION_CONVERTER; + private static final Converter, Long> STRING_LIST_TO_TIME_CONVERTER; public static final byte[] PLUS_BYTES; public static final byte[] MINUS_BYTES; @@ -165,6 +166,19 @@ abstract public class JedisConverters extends Converters { } }; + + STRING_LIST_TO_TIME_CONVERTER = new Converter, Long>() { + + @Override + public Long convert(List source) { + + Assert.notEmpty(source, "Received invalid result from server. Expected 2 items in collection."); + Assert.isTrue(source.size() == 2, + "Received invalid nr of arguments from redis server. Expected 2 received " + source.size()); + + return toTimeMillis(source.get(0), source.get(1)); + } + }; } public static Converter stringToBytes() { @@ -173,7 +187,7 @@ abstract public class JedisConverters extends Converters { /** * {@link ListConverter} converting jedis {@link redis.clients.jedis.Tuple} to {@link Tuple}. - * + * * @return * @since 1.4 */ @@ -343,7 +357,7 @@ abstract public class JedisConverters extends Converters { /** * Converts a given {@link Boundary} to its binary representation suitable for {@literal ZRANGEBY*} commands, despite * {@literal ZRANGEBYLEX}. - * + * * @param boundary * @param defaultValue * @return @@ -360,7 +374,7 @@ abstract public class JedisConverters extends Converters { /** * Converts a given {@link Boundary} to its binary representation suitable for ZRANGEBYLEX command. - * + * * @param boundary * @return * @since 1.6 @@ -382,7 +396,7 @@ abstract public class JedisConverters extends Converters { *
{@link TimeUnit#MILLISECONDS}
*
{@code PX}
* - * + * * @param expiration * @return * @since 1.7 @@ -401,7 +415,7 @@ abstract public class JedisConverters extends Converters { *
{@link SetOption#SET_IF_PRESENT}
*
{@code XX}
* - * + * * @param option * @return * @since 1.7 @@ -442,9 +456,9 @@ abstract public class JedisConverters extends Converters { * @return */ public static ScanParams toScanParams(ScanOptions options) { - + ScanParams sp = new ScanParams(); - + if (!options.equals(ScanOptions.NONE)) { if (options.getCount() != null) { sp.count(options.getCount().intValue()); @@ -456,4 +470,8 @@ abstract public class JedisConverters extends Converters { return sp; } + static Converter, Long> toTimeConverter() { + return STRING_LIST_TO_TIME_CONVERTER; + } + } diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java index d7b312ca2..843e4b11a 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java @@ -54,7 +54,6 @@ import org.springframework.data.redis.connection.RedisSubscribedConnectionExcept import org.springframework.data.redis.connection.ReturnType; import org.springframework.data.redis.connection.SortParameters; import org.springframework.data.redis.connection.Subscription; -import org.springframework.data.redis.connection.convert.Converters; import org.springframework.data.redis.connection.convert.TransactionResultConverter; import org.springframework.data.redis.core.Cursor; import org.springframework.data.redis.core.KeyBoundCursor; @@ -111,7 +110,7 @@ import com.lambdaworks.redis.pubsub.RedisPubSubConnection; /** * {@code RedisConnection} implementation on top of Lettuce Redis * client. - * + * * @author Costin Leau * @author Jennifer Hickey * @author Christoph Strobl @@ -254,7 +253,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Instantiates a new lettuce connection. - * + * * @param timeout The connection timeout (in milliseconds) * @param client The {@link RedisClient} to use when instantiating a native connection */ @@ -264,7 +263,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Instantiates a new lettuce connection. - * + * * @param timeout The connection timeout (in milliseconds) * @param client The {@link RedisClient} to use when * instantiating a pub/sub connection * @param pool The connection pool to use for all other native connections @@ -275,7 +274,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Instantiates a new lettuce connection. - * + * * @param sharedConnection A native connection that is shared with other {@link LettuceConnection}s. Will not be used * for transactions or blocking operations * @param timeout The connection timeout (in milliseconds) @@ -288,7 +287,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Instantiates a new lettuce connection. - * + * * @param sharedConnection A native connection that is shared with other {@link LettuceConnection}s. Should not be * used for transactions or blocking operations * @param timeout The connection timeout (in milliseconds) @@ -347,7 +346,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * 'Native' or 'raw' execution of the given command along-side the given arguments. - * + * * @see RedisCommands#execute(String, byte[]...) * @param command Command to execute * @param commandOutputTypeHint Type of Output to use, may be (may be {@literal null}). @@ -3085,16 +3084,19 @@ public class LettuceConnection extends AbstractRedisConnection { */ @Override public Long time() { + try { - List result = getConnection().time(); - - Assert.notEmpty(result, "Received invalid result from server. Expected 2 items in collection."); - Assert.isTrue(result.size() == 2, "Received invalid nr of arguments from redis server. Expected 2 received " - + result.size()); - - return Converters.toTimeMillis(new String(result.get(0)), new String(result.get(1))); + if (isPipelined()) { + pipeline(new LettuceResult(getAsyncConnection().time(), LettuceConverters.toTimeConverter())); + return null; + } + if (isQueueing()) { + transaction(new LettuceTxResult(getConnection().time(), LettuceConverters.toTimeConverter())); + return null; + } + return LettuceConverters.toTimeConverter().convert(getConnection().time()); } catch (Exception ex) { throw convertLettuceAccessException(ex); } @@ -3406,7 +3408,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Specifies if pipelined and transaction results should be converted to the expected data type. If false, results of * {@link #closePipeline()} and {@link #exec()} will be of the type returned by the Lettuce driver - * + * * @param convertPipelineAndTxResults Whether or not to convert pipeline and tx results */ public void setConvertPipelineAndTxResults(boolean convertPipelineAndTxResults) { @@ -3643,7 +3645,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * {@link TypeHints} provide {@link CommandOutput} information for a given {@link CommandType}. - * + * * @since 1.2.1 */ static class TypeHints { @@ -3804,7 +3806,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Returns the {@link CommandOutput} mapped for given {@link CommandType} or {@link ByteArrayOutput} as default. - * + * * @param type * @return {@link ByteArrayOutput} as default when no matching {@link CommandOutput} available. */ @@ -3815,7 +3817,7 @@ public class LettuceConnection extends AbstractRedisConnection { /** * Returns the {@link CommandOutput} mapped for given {@link CommandType} given {@link CommandOutput} as default. - * + * * @param type * @return */ diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java index 9d57b17c2..39fa2aee3 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java @@ -67,7 +67,7 @@ import com.lambdaworks.redis.protocol.SetArgs; /** * Lettuce type converters - * + * * @author Jennifer Hickey * @author Christoph Strobl * @author Thomas Darimont @@ -91,6 +91,7 @@ abstract public class LettuceConverters extends Converters { private static final Converter> STRING_TO_LIST_OF_CLIENT_INFO = new StringToRedisClientInfoConverter(); private static final Converter> PARTITIONS_TO_CLUSTER_NODES; private static Converter CLUSTER_NODE_TO_CLUSTER_NODE_CONVERTER; + private static final Converter, Long> BYTES_LIST_TO_TIME_CONVERTER; public static final byte[] PLUS_BYTES; public static final byte[] MINUS_BYTES; @@ -284,6 +285,19 @@ abstract public class LettuceConverters extends Converters { MINUS_BYTES = toBytes("-"); POSITIVE_INFINITY_BYTES = toBytes("+inf"); NEGATIVE_INFINITY_BYTES = toBytes("-inf"); + + BYTES_LIST_TO_TIME_CONVERTER = new Converter, Long>() { + + @Override + public Long convert(List source) { + + Assert.notEmpty(source, "Received invalid result from server. Expected 2 items in collection."); + Assert.isTrue(source.size() == 2, + "Received invalid nr of arguments from redis server. Expected 2 received " + source.size()); + + return toTimeMillis(LettuceConverters.toString(source.get(0)), LettuceConverters.toString(source.get(1))); + } + }; } public static List toTuple(List list) { @@ -620,7 +634,7 @@ abstract public class LettuceConverters extends Converters { /** * Converts a given {@link Expiration} and {@link SetOption} to the according {@link SetArgs}.
- * + * * @param expiration can be {@literal null}. * @param option can be {@literal null}. * @since 1.7 @@ -657,4 +671,7 @@ abstract public class LettuceConverters extends Converters { return args; } + static Converter, Long> toTimeConverter() { + return BYTES_LIST_TO_TIME_CONVERTER; + } } diff --git a/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java index 513281a53..3477b9a28 100644 --- a/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java @@ -15,8 +15,7 @@ */ package org.springframework.data.redis.connection; -import static org.hamcrest.CoreMatchers.*; -import static org.hamcrest.number.IsCloseTo.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.junit.Assume.*; import static org.springframework.data.redis.SpinBarrier.*; @@ -76,7 +75,7 @@ import org.springframework.test.annotation.ProfileValueSourceConfiguration; /** * Base test class for AbstractConnection integration tests - * + * * @author Costin Leau * @author Jennifer Hickey * @author Christoph Strobl @@ -1922,13 +1921,17 @@ public abstract class AbstractConnectionIntegrationTests { /** * @see DATAREDIS-206 + * @see DATAREDIS-513 */ @Test public void testGetTimeShouldRequestServerTime() { - Long time = connectionFactory.getConnection().time(); - assertThat(time, notNullValue()); - assertThat(time > 0, equalTo(true)); + actual.add(connection.time()); + + List results = getResults(); + assertThat(results, is(not(empty()))); + assertThat(results.get(0), notNullValue()); + assertThat((Long) results.get(0) > 0, equalTo(true)); } /**