From 9250063acff0c64b461200a6fe0bc1beb1186b8a Mon Sep 17 00:00:00 2001 From: Jennifer Hickey Date: Thu, 13 Jun 2013 14:48:22 -0700 Subject: [PATCH] Upgrade to SRP 0.6 DATAREDIS-186 - Handle a few method signature changes to varargs - Handle possibility of exec() throwing Exceptions if ErrorReply received in Redis 2.6 --- gradle.properties | 2 +- .../redis/connection/srp/SrpConnection.java | 36 +++++++++++++------ .../data/redis/RedisVersionUtils.java | 4 +-- .../srp/SrpConnectionIntegrationTests.java | 8 ----- ...SrpConnectionPipelineIntegrationTests.java | 23 ++++++++++-- ...pConnectionPipelineTxIntegrationTests.java | 33 ++++++++++------- 6 files changed, 69 insertions(+), 37 deletions(-) diff --git a/gradle.properties b/gradle.properties index 6cf3964bd..07ebe9537 100644 --- a/gradle.properties +++ b/gradle.properties @@ -6,7 +6,7 @@ jedisVersion=2.1.0 springVersion=3.1.4.RELEASE log4jVersion=1.2.17 version=1.0.5.BUILD-SNAPSHOT -srpVersion=0.5 +srpVersion=0.6 jacksonVersion=1.8.8 lettuceVersion=2.3.2 mockitoVersion=1.8.5 diff --git a/src/main/java/org/springframework/data/redis/connection/srp/SrpConnection.java b/src/main/java/org/springframework/data/redis/connection/srp/SrpConnection.java index 98370c675..8906f0cbf 100644 --- a/src/main/java/org/springframework/data/redis/connection/srp/SrpConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/srp/SrpConnection.java @@ -427,17 +427,31 @@ public class SrpConnection implements RedisConnection { public List exec() { isMulti = false; + Exception execException = null; + List results = null; try { Future exec = client.exec(); - // Need to wait on execution or subsequent non-pipelined calls like multi may read exec results + // Need to wait on execution or subsequent non-pipelined calls may read exec results exec.get(); - if (pipelineRequested) { - return null; - } - return closePipeline(); } catch (Exception ex) { - throw convertSrpAccessException(ex); + execException = ex; + } finally { + // ensure pipeline is closed even if error in exec + if (!pipelineRequested) { + try { + results = closePipeline(); + } catch (RedisPipelineException e) { + if(execException == null) { + execException = e; + } + } + } } + if(execException != null) { + // Intentionally convert RedisPipelineException too, it's an impl detail + throw convertSrpAccessException(execException); + } + return results; } @@ -1076,7 +1090,7 @@ public class SrpConnection implements RedisConnection { pipeline(pipeline.brpoplpush(srcKey, dstKey, timeout)); return null; } - return client.brpoplpush(srcKey, dstKey, timeout).data(); + return (byte[]) client.brpoplpush(srcKey, dstKey, timeout).data(); } catch (Exception ex) { throw convertSrpAccessException(ex); } @@ -1360,10 +1374,10 @@ public class SrpConnection implements RedisConnection { public Long zInterStore(byte[] destKey, byte[]... sets) { try { if (isPipelined()) { - pipeline(pipeline.zinterstore(destKey, sets.length, sets)); + pipeline(pipeline.zinterstore(destKey, sets.length, (Object[]) sets)); return null; } - return client.zinterstore(destKey, sets.length, sets).data(); + return client.zinterstore(destKey, sets.length, (Object[]) sets).data(); } catch (Exception ex) { throw convertSrpAccessException(ex); } @@ -1762,10 +1776,10 @@ public class SrpConnection implements RedisConnection { public void hMSet(byte[] key, Map tuple) { try { if (isPipelined()) { - pipeline(pipeline.hmset(key, SrpUtils.convert(tuple))); + pipeline(pipeline.hmset(key, (Object[]) SrpUtils.convert(tuple))); return; } - client.hmset(key, SrpUtils.convert(tuple)); + client.hmset(key, (Object[]) SrpUtils.convert(tuple)); } catch (Exception ex) { throw convertSrpAccessException(ex); } diff --git a/src/test/java/org/springframework/data/redis/RedisVersionUtils.java b/src/test/java/org/springframework/data/redis/RedisVersionUtils.java index 5ee4287c0..956560f8a 100644 --- a/src/test/java/org/springframework/data/redis/RedisVersionUtils.java +++ b/src/test/java/org/springframework/data/redis/RedisVersionUtils.java @@ -49,8 +49,8 @@ public abstract class RedisVersionUtils { String major = matcher.group(1); String minor = matcher.group(2); String patch = matcher.group(4); - return new Version(Integer.parseInt(major), Integer.parseInt(minor), - Integer.parseInt(patch)); + return new Version(Integer.parseInt(major), minor != null ? Integer.parseInt(minor) : 0, + patch != null ? Integer.parseInt(patch) : 0); } throw new IllegalArgumentException("Specified version cannot be parsed"); } diff --git a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionIntegrationTests.java index 6069ad6be..967eb813d 100644 --- a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionIntegrationTests.java @@ -54,18 +54,10 @@ public class SrpConnectionIntegrationTests extends AbstractConnectionIntegration connection = null; } - @Ignore("DATAREDIS-169 SRP discard does not clear txReplies, results in inconsistent results on next tx exec") - public void testMultiDiscard() { - } - @Ignore("DATAREDIS-168 SRP exec throws TransactionFailedException if watched value modified") public void testWatch() { } - @Ignore("DATAREDIS-156 SRP bRPopLPush ClassCastException") - public void testBRPopLPushTimeout() { - } - @Test(expected = UnsupportedOperationException.class) public void testZInterStoreAggWeights() { super.testZInterStoreAggWeights(); diff --git a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineIntegrationTests.java index 5b9bbff6d..440cee3c9 100644 --- a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineIntegrationTests.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeTrue; +import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.Arrays; @@ -33,7 +34,9 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.data.redis.RedisVersionUtils; import org.springframework.data.redis.connection.AbstractConnectionPipelineIntegrationTests; +import org.springframework.data.redis.connection.DefaultStringRedisConnection; import org.springframework.data.redis.connection.DefaultStringTuple; +import org.springframework.data.redis.connection.RedisPipelineException; import org.springframework.data.redis.connection.RedisZSetCommands.Tuple; import org.springframework.data.redis.connection.StringRedisConnection.StringTuple; import org.springframework.data.redis.serializer.SerializationUtils; @@ -54,8 +57,24 @@ import redis.reply.Reply; public class SrpConnectionPipelineIntegrationTests extends AbstractConnectionPipelineIntegrationTests { - @Ignore("DATAREDIS-169 SRP discard does not clear txReplies, results in inconsistent results on next tx exec") - public void testMultiDiscard() { + @Test + public void testMultiDiscard() throws Exception { + DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( + connectionFactory.getConnection()); + conn2.set("testitnow", "willdo"); + connection.multi(); + connection.set("testitnow2", "notok"); + connection.discard(); + // SRP throws Exception on transaction discard, so we expect pipeline exception here + try { + getResults(); + fail("Closing the pipeline on a discarded tx should throw a RedisPipelineException"); + }catch(RedisPipelineException e) { + } + assertEquals("willdo", connection.get("testitnow")); + connection.openPipeline(); + // Ensure we can run a new tx after discarding previous one + testMultiExec(); } @Ignore("DATAREDIS-168 SRP exec throws TransactionFailedException if watched value modified") diff --git a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineTxIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineTxIntegrationTests.java index 4c28c642d..03aeb75aa 100644 --- a/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineTxIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/srp/SrpConnectionPipelineTxIntegrationTests.java @@ -17,13 +17,13 @@ package org.springframework.data.redis.connection.srp; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; -import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; - +import static org.junit.Assert.fail; import java.util.Arrays; import java.util.List; import org.junit.Test; +import org.springframework.data.redis.RedisSystemException; import org.springframework.data.redis.RedisVersionUtils; import org.springframework.data.redis.connection.RedisPipelineException; @@ -33,23 +33,30 @@ import org.springframework.data.redis.connection.RedisPipelineException; * @author Jennifer Hickey * */ -public class SrpConnectionPipelineTxIntegrationTests extends - SrpConnectionTransactionIntegrationTests { +public class SrpConnectionPipelineTxIntegrationTests extends SrpConnectionTransactionIntegrationTests { @Test public void exceptionExecuteNative() throws Exception { - // DATAREDIS-172 SRP ClassCastException when exec() returns an - // ErrorReply in Redis 2.6 - connection.exec(); - connection.closePipeline(); - boolean execErrorSupported = RedisVersionUtils.atMost("2.6.4", connection); - initConnection(); - assumeTrue(execErrorSupported); connection.execute("ZadD", getClass() + "#foo\t0.90\titem"); try { getResults(); - fail("Execute failures should result in a RedisPipelineException"); - } catch (RedisPipelineException e) { + fail("Expected an Exception to be thrown executing a command with syntax error"); + }catch(RedisPipelineException e) { + // Redis 2.4, Exception occurs when we get the result of execute on closePipeline + if(RedisVersionUtils.atLeast("2.6.4", byteConnection)) { + fail("RedisPipelineException should not be thrown in Redis 2.6"); + } + }catch(RedisSystemException e) { + try { + connection.closePipeline(); + }catch(Exception ex) { + // Gonna get another error on closing the pipeline as results from execute + } + if(!RedisVersionUtils.atLeast("2.6", byteConnection)) { + // Redis 2.6 returns an ErrorReploy on exec, the Exception occurs when we call exec() + // b/c it waits on the response + fail("RedisSystemException should only be thrown in Redis 2.6"); + } } }