From bfc20834133323be9f4e5ac307d81b7901f9367a Mon Sep 17 00:00:00 2001 From: Matt Hall Date: Thu, 24 Jan 2013 18:09:44 -0500 Subject: [PATCH] Force checked exception on redis connection failure so cache implementation can handle it appropriately. --- app/controllers/RedisCacheMonitor.java | 6 +- conf/dependencies.yml | 2 +- src/play/modules/redis/RedisCacheImpl.java | 197 +++++++++++++-------- src/play/modules/redis/RedisPlugin.java | 8 +- 4 files changed, 135 insertions(+), 78 deletions(-) diff --git a/app/controllers/RedisCacheMonitor.java b/app/controllers/RedisCacheMonitor.java index 3e53772..4f9b117 100644 --- a/app/controllers/RedisCacheMonitor.java +++ b/app/controllers/RedisCacheMonitor.java @@ -12,7 +12,7 @@ public class RedisCacheMonitor extends Controller { - public static void cacheInstance() { + public static void cacheInstance() throws RedisCacheImpl.JedisCheckedException { if (RedisPlugin.isRedisCacheEnabled()) { Map cacheInstanceInfo = new HashMap(2); cacheInstanceInfo.put("host", RedisCacheImpl.getCacheConnection().getClient().getHost()); @@ -21,7 +21,7 @@ public static void cacheInstance() { } } - public static void cacheClientInfo() { + public static void cacheClientInfo() throws RedisCacheImpl.JedisCheckedException { if (RedisPlugin.isRedisCacheEnabled()) { String clientInfo = RedisCacheImpl.getCacheConnection().info(); @@ -41,7 +41,7 @@ public static void cacheClientInfo() { } } - public static void cacheContents() { + public static void cacheContents() throws RedisCacheImpl.JedisCheckedException { if (RedisPlugin.isRedisCacheEnabled()) { Set keys = RedisCacheImpl.getCacheConnection().keys("*"); diff --git a/conf/dependencies.yml b/conf/dependencies.yml index 3df6821..c6863c7 100644 --- a/conf/dependencies.yml +++ b/conf/dependencies.yml @@ -1,4 +1,4 @@ -self: play -> redis 0.3 +self: play -> redis 0.4 require: - play diff --git a/src/play/modules/redis/RedisCacheImpl.java b/src/play/modules/redis/RedisCacheImpl.java index 985318c..a6d7c5f 100644 --- a/src/play/modules/redis/RedisCacheImpl.java +++ b/src/play/modules/redis/RedisCacheImpl.java @@ -1,10 +1,12 @@ package play.modules.redis; +import play.Logger; import play.Play; import play.cache.CacheImpl; import redis.clients.jedis.Jedis; import redis.clients.jedis.JedisPool; import redis.clients.jedis.exceptions.JedisDataException; +import redis.clients.jedis.exceptions.JedisException; import java.io.*; import java.util.HashMap; @@ -28,29 +30,47 @@ static RedisCacheImpl getInstance() { return uniqueInstance; } - public static Jedis getCacheConnection() { - if (cacheConnection.get() != null) { - return cacheConnection.get(); - } - - Jedis connection = connectionPool.getResource(); - cacheConnection.set(connection); - return connection; + public static class JedisCheckedException extends Exception { + + } + + public static Jedis getCacheConnection() throws JedisCheckedException { + try { + if (cacheConnection.get() != null) { + return cacheConnection.get(); + } + + Jedis connection = connectionPool.getResource(); + cacheConnection.set(connection); + return connection; + } catch (JedisException e) { + Logger.warn(e, e.getMessage()); + throw new JedisCheckedException(); + } } - public static void closeCacheConnection() { - if (cacheConnection.get() != null) { - connectionPool.returnResource(cacheConnection.get()); - cacheConnection.remove(); + public static void closeCacheConnection() throws JedisCheckedException { + try { + if (cacheConnection.get() != null) { + connectionPool.returnResource(cacheConnection.get()); + cacheConnection.remove(); + } + } catch (JedisException e) { + Logger.warn(e, e.getMessage()); + throw new JedisCheckedException(); } } @Override public void add(String key, Object value, int expiration) { - if (!getCacheConnection().exists(key)) { - set(key, value, expiration); - } - } + try { + if (!getCacheConnection().exists(key)) { + set(key, value, expiration); + } + } catch (JedisCheckedException e) { + Logger.warn("Unable to add " + key + " to redis cache due to previous exception."); + } + } @Override public boolean safeAdd(String key, Object value, int expiration) { @@ -64,14 +84,18 @@ public boolean safeAdd(String key, Object value, int expiration) { @Override public void set(String key, Object value, int expiration) { - Jedis jedis = getCacheConnection(); - - // Serialize to a byte array - byte[] bytes = toByteArray(value); - - jedis.set(key.getBytes(), bytes); - jedis.expire(key, expiration); - } + try { + Jedis jedis = getCacheConnection(); + + // Serialize to a byte array + byte[] bytes = toByteArray(value); + + jedis.set(key.getBytes(), bytes); + jedis.expire(key, expiration); + } catch (JedisCheckedException e) { + Logger.warn("Unable to set " + key + " to redis cache due to previous exception."); + } + } private static byte[] toByteArray(Object o) { ObjectOutputStream out = null; @@ -104,10 +128,14 @@ public boolean safeSet(String key, Object value, int expiration) { @Override public void replace(String key, Object value, int expiration) { - if (getCacheConnection().exists(key)) { - set(key, value, expiration); - } - } + try { + if (getCacheConnection().exists(key)) { + set(key, value, expiration); + } + } catch (JedisCheckedException e) { + Logger.error(e, e.getMessage()); + } + } @Override public boolean safeReplace(String key, Object value, int expiration) { @@ -121,11 +149,16 @@ public boolean safeReplace(String key, Object value, int expiration) { @Override public Object get(String key) { - byte[] bytes = getCacheConnection().get(key.getBytes()); - if (bytes == null) return null; + try { + byte[] bytes = getCacheConnection().get(key.getBytes()); + if (bytes == null) return null; - return fromByteArray(bytes); - } + return fromByteArray(bytes); + } catch (JedisCheckedException e) { + Logger.warn("Unable to get " + key + " to redis cache due to previous exception, returning null."); + return null; + } + } private static Object fromByteArray(byte[] bytes) { @@ -163,58 +196,74 @@ public Map get(String[] keys) { public long incr(String key, int by) { Object cacheValue = get(key); long sum; - if (cacheValue == null) { - Long newCacheValueLong = Long.valueOf(0L + by); - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); - sum = newCacheValueLong.longValue(); - } else if (cacheValue instanceof Integer) { - Integer newCacheValueInteger = (Integer)cacheValue + by; - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueInteger)); - sum = newCacheValueInteger.longValue(); - } else if (cacheValue instanceof Long) { - Long newCacheValueLong = (Long)cacheValue + by; - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); - sum = newCacheValueLong.longValue(); - } else { - throw new JedisDataException("Cannot incr on non-integer value (key: " + key + ")"); - } - - return sum; + try { + if (cacheValue == null) { + Long newCacheValueLong = Long.valueOf(0L + by); + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); + sum = newCacheValueLong.longValue(); + } else if (cacheValue instanceof Integer) { + Integer newCacheValueInteger = (Integer)cacheValue + by; + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueInteger)); + sum = newCacheValueInteger.longValue(); + } else if (cacheValue instanceof Long) { + Long newCacheValueLong = (Long)cacheValue + by; + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); + sum = newCacheValueLong.longValue(); + } else { + throw new JedisDataException("Cannot incr on non-integer value (key: " + key + ")"); + } + } catch (JedisCheckedException e) { + throw new JedisException("Cannot incr due to server problem (key: " + key + ")"); + } + + return sum; } @Override public long decr(String key, int by) { Object cacheValue = get(key); long difference; - if (cacheValue == null) { - Long newCacheValueLong = Long.valueOf(0L - by); - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); - difference = newCacheValueLong.longValue(); - } else if (cacheValue instanceof Integer) { - Integer newCacheValueInteger = (Integer)cacheValue - by; - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueInteger)); - difference = newCacheValueInteger.longValue(); - } else if (cacheValue instanceof Long) { - Long newCacheValueLong = (Long)cacheValue - by; - getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); - difference = newCacheValueLong.longValue(); - } else { - throw new JedisDataException("Cannot decr on non-integer value (key: " + key + ")"); - } - - return difference; + try { + if (cacheValue == null) { + Long newCacheValueLong = Long.valueOf(0L - by); + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); + difference = newCacheValueLong.longValue(); + } else if (cacheValue instanceof Integer) { + Integer newCacheValueInteger = (Integer)cacheValue - by; + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueInteger)); + difference = newCacheValueInteger.longValue(); + } else if (cacheValue instanceof Long) { + Long newCacheValueLong = (Long)cacheValue - by; + getCacheConnection().set(key.getBytes(), toByteArray(newCacheValueLong)); + difference = newCacheValueLong.longValue(); + } else { + throw new JedisDataException("Cannot decr on non-integer value (key: " + key + ")"); + } + } catch (JedisCheckedException e) { + throw new JedisException("Cannot decr due to server problem (key: " + key + ")"); + } + + return difference; } @Override public void clear() { - getCacheConnection().flushDB(); - // TODO: check return status code + try { + getCacheConnection().flushDB(); + } catch (JedisCheckedException e) { + Logger.error(e, "Couldn't clear cache due to error: " + e.getMessage()); + } + // TODO: check return status code } @Override public void delete(String key) { - getCacheConnection().del(key); - // TODO: check return status code + try { + getCacheConnection().del(key); + } catch (JedisCheckedException e) { + Logger.error(e, "Couldn't delete key " + key + " due to error: " + e.getMessage()); + } + // TODO: check return status code } @Override @@ -229,8 +278,12 @@ public boolean safeDelete(String key) { @Override public void stop() { - getCacheConnection().flushAll(); - connectionPool.destroy(); + try { + getCacheConnection().flushAll(); + } catch (JedisCheckedException e) { + Logger.error(e, e.getMessage()); + } + connectionPool.destroy(); } } diff --git a/src/play/modules/redis/RedisPlugin.java b/src/play/modules/redis/RedisPlugin.java index 5358915..6a7fe95 100644 --- a/src/play/modules/redis/RedisPlugin.java +++ b/src/play/modules/redis/RedisPlugin.java @@ -84,8 +84,12 @@ public void onApplicationStop() { @Override public void invocationFinally() { - if (createdRedisCache) RedisCacheImpl.closeCacheConnection(); - if (createdRedis) RedisConnectionManager.closeConnection(); + try { + if (createdRedisCache) RedisCacheImpl.closeCacheConnection(); + } catch (RedisCacheImpl.JedisCheckedException e) { + Logger.error(e, e.getMessage()); + } + if (createdRedis) RedisConnectionManager.closeConnection(); } private static class RedisConnectionInfo {