You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ds...@apache.org on 2020/04/28 18:24:48 UTC
[geode] branch develop updated: GEODE 8014: delete redis sets and
hashes when empty (#4989)
This is an automated email from the ASF dual-hosted git repository.
dschneider pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 3aa3659 GEODE 8014: delete redis sets and hashes when empty (#4989)
3aa3659 is described below
commit 3aa36591d2c5f0610f5a819cdd4552037caf1c0b
Author: Sarah Abbey <41...@users.noreply.github.com>
AuthorDate: Tue Apr 28 14:24:19 2020 -0400
GEODE 8014: delete redis sets and hashes when empty (#4989)
---
.../apache/geode/redis/HashesIntegrationTest.java | 79 ++++++++++++++++++++++
.../apache/geode/redis/SetsIntegrationTest.java | 77 +++++++++++++++++++++
.../org/apache/geode/redis/GeodeRedisServer.java | 10 +++
.../geode/redis/internal/RedisConstants.java | 2 +-
.../executor/hash/GeodeRedisHashSynchronized.java | 7 ++
.../redis/internal/executor/hash/HDelExecutor.java | 2 +
.../executor/set/GeodeRedisSetSynchronized.java | 10 +++
.../redis/internal/executor/set/SRemExecutor.java | 1 +
.../executor/string/GetSetExecutorJUnitTest.java | 21 ------
9 files changed, 187 insertions(+), 22 deletions(-)
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
index 98f7486..dfe7fd7 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/HashesIntegrationTest.java
@@ -38,7 +38,12 @@ import java.util.Map.Entry;
import java.util.Random;
import java.util.Set;
import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicLong;
import org.apache.commons.lang3.RandomStringUtils;
@@ -47,6 +52,7 @@ import org.junit.After;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import redis.clients.jedis.Jedis;
@@ -57,6 +63,7 @@ import org.apache.geode.cache.CacheFactory;
import org.apache.geode.cache.GemFireCache;
import org.apache.geode.internal.AvailablePortHelper;
import org.apache.geode.redis.general.ConcurrentLoopingThreads;
+import org.apache.geode.redis.internal.ByteArrayWrapper;
import org.apache.geode.test.junit.categories.RedisTest;
@Category({RedisTest.class})
@@ -146,6 +153,78 @@ public class HashesIntegrationTest {
assertTrue(jedis.hlen(key) == 0);
}
+ @Test
+ public void testHDelErrorMessage_givenIncorrectDataType() {
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> {
+ jedis.hdel("farm", "chicken");
+ }).isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the wrong kind of value");
+ }
+
+ @Test
+ public void testHDelDeletesKeyWhenHashIsEmpty() {
+ jedis.hset("farm", "chicken", "little");
+
+ jedis.hdel("farm", "chicken");
+
+ assertThat(jedis.exists("farm")).isFalse();
+ }
+
+ @Ignore("GEODE-7905")
+ @Test
+ public void testConcurrentHDelConsistentlyUpdatesMetaInformation()
+ throws ExecutionException, InterruptedException {
+ ByteArrayWrapper keyAsByteArray = new ByteArrayWrapper("hash".getBytes());
+ AtomicLong errorCount = new AtomicLong();
+ CyclicBarrier startCyclicBarrier = new CyclicBarrier(2, () -> {
+ boolean keyIsRegistered = server.getKeyRegistrar().isRegistered(keyAsByteArray);
+ boolean containsKey = server.getRegionCache().getHashRegion().containsKey(keyAsByteArray);
+
+ if (keyIsRegistered != containsKey) {
+ errorCount.getAndIncrement();
+ jedis.hset("hash", "field", "value");
+ jedis.del("hash");
+ }
+ });
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+
+ Callable<Long> callable1 = () -> {
+ Long removedCount = 0L;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ Long result = jedis.hdel("hash", "field");
+ startCyclicBarrier.await();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ return removedCount;
+ };
+
+ Callable<Long> callable2 = () -> {
+ Long addedCount = 0L;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ addedCount += jedis2.hset("hash", "field", "value");
+ startCyclicBarrier.await();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ return addedCount;
+ };
+
+ Future<Long> future1 = pool.submit(callable1);
+ Future<Long> future2 = pool.submit(callable2);
+
+ future1.get();
+ future2.get();
+
+ assertThat(errorCount.get())
+ .as("Inconsistency between keyRegistrar and backing store detected.").isEqualTo(0L);
+ }
@Test
public void testHkeys() {
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/SetsIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/SetsIntegrationTest.java
index 7d8da16..61d7305 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/SetsIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/SetsIntegrationTest.java
@@ -18,6 +18,7 @@ import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.catchThrowable;
import java.util.ArrayList;
@@ -25,6 +26,7 @@ import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -34,6 +36,7 @@ import java.util.concurrent.atomic.AtomicLong;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
+import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
@@ -45,6 +48,7 @@ import org.apache.geode.cache.CacheFactory;
import org.apache.geode.cache.GemFireCache;
import org.apache.geode.internal.AvailablePortHelper;
import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.redis.internal.ByteArrayWrapper;
import org.apache.geode.redis.internal.RedisConstants;
import org.apache.geode.test.junit.categories.RedisTest;
@@ -213,6 +217,79 @@ public class SetsIntegrationTest {
}
@Test
+ public void testSRemErrorMessage_givenIncorrectDataType() {
+ jedis.set("farm", "chicken");
+ assertThatThrownBy(() -> {
+ jedis.srem("farm", "chicken");
+ }).isInstanceOf(JedisDataException.class)
+ .hasMessageContaining("WRONGTYPE Operation against a key holding the wrong kind of value");
+ }
+
+ @Test
+ public void testSRemDeletesKeyWhenSetIsEmpty() {
+ jedis.sadd("farm", "chicken");
+
+ jedis.srem("farm", "chicken");
+
+ assertThat(jedis.exists("farm")).isFalse();
+ }
+
+ @Ignore("GEODE-7905")
+ @Test
+ public void testConcurrentSRemConsistentlyUpdatesMetaInformation()
+ throws ExecutionException, InterruptedException {
+ ByteArrayWrapper keyAsByteArray = new ByteArrayWrapper("set".getBytes());
+ AtomicLong errorCount = new AtomicLong();
+ CyclicBarrier startCyclicBarrier = new CyclicBarrier(2, () -> {
+ boolean keyIsRegistered = server.getKeyRegistrar().isRegistered(keyAsByteArray);
+ boolean containsKey = server.getRegionCache().getSetRegion().containsKey(keyAsByteArray);
+
+ if (keyIsRegistered != containsKey) {
+ errorCount.getAndIncrement();
+ jedis.sadd("set", "member");
+ jedis.del("set");
+ }
+ });
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+
+ Callable<Long> callable1 = () -> {
+ Long removedCount = 0L;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ Long result = jedis.srem("set", "member");
+ startCyclicBarrier.await();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ return removedCount;
+ };
+
+ Callable<Long> callable2 = () -> {
+ Long addedCount = 0L;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ addedCount += jedis2.sadd("set", "member");
+ startCyclicBarrier.await();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ return addedCount;
+ };
+
+ Future<Long> future1 = pool.submit(callable1);
+ Future<Long> future2 = pool.submit(callable2);
+
+ future1.get();
+ future2.get();
+
+ assertThat(errorCount.get())
+ .as("Inconsistency between keyRegistrar and backing store detected.").isEqualTo(0L);
+ }
+
+ @Test
public void testSMembersSIsMember() {
int elements = 10;
Set<String> strings = new HashSet<String>();
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/GeodeRedisServer.java b/geode-redis/src/main/java/org/apache/geode/redis/GeodeRedisServer.java
index 58f634e..a8c0f48 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/GeodeRedisServer.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/GeodeRedisServer.java
@@ -294,10 +294,15 @@ public class GeodeRedisServer {
private boolean shutdown;
private boolean started;
+
private KeyRegistrar keyRegistrar;
private PubSub pubSub;
private RedisLockService hashLockService;
+ @VisibleForTesting
+ protected KeyRegistrar getKeyRegistrar() {
+ return keyRegistrar;
+ }
/**
* Determine the {@link RegionShortcut} type from a String value. If the String value doesn't map
@@ -450,6 +455,11 @@ public class GeodeRedisServer {
logger = cache.getLogger();
}
+ @VisibleForTesting
+ protected RegionProvider getRegionCache() {
+ return regionCache;
+ }
+
private void initializeRedis() {
synchronized (cache) {
Region<ByteArrayWrapper, ByteArrayWrapper> stringsRegion;
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
index 0f64cb5..e3d4621 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
@@ -64,7 +64,7 @@ public class RedisConstants {
public static final String ERROR_NOT_AUTH = "Must authenticate before sending any requests";
public static final String ERROR_ZSET_MEMBER_NOT_FOUND = "could not decode requested zset member";
public static final String ERROR_WRONG_TYPE =
- "WRONGTYPE Operation against a key holding the wrong kind of value";
+ "Operation against a key holding the wrong kind of value";
public static final String ERROR_NOT_INTEGER = "value is not an integer or out of range";
public static final String ERROR_OVERFLOW = "increment or decrement would overflow";
public static final String ERROR_NO_SUCH_KEY = "no such key";
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
index 49a27bb..7dffc52 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
@@ -89,6 +89,13 @@ class GeodeRedisHashSynchronized implements RedisHash {
numDeleted.set(oldHash.size() - newHash.size());
return newHash;
});
+
+ if (hgetall().isEmpty()) {
+ RedisDataType type = context.getKeyRegistrar().getType(key);
+ if (type == RedisDataType.REDIS_HASH) {
+ context.getRegionProvider().removeKey(key, type);
+ }
+ }
return numDeleted.intValue();
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java
index 0606c08..b891eae 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HDelExecutor.java
@@ -21,6 +21,7 @@ import org.apache.geode.redis.internal.Coder;
import org.apache.geode.redis.internal.Command;
import org.apache.geode.redis.internal.ExecutionHandlerContext;
import org.apache.geode.redis.internal.RedisConstants.ArityDef;
+import org.apache.geode.redis.internal.RedisDataType;
/**
* <pre>
@@ -55,6 +56,7 @@ public class HDelExecutor extends HashExecutor {
ByteArrayWrapper key = command.getKey();
+ checkDataType(key, RedisDataType.REDIS_HASH, context);
RedisHash hash = new GeodeRedisHashSynchronized(key, context);
int numDeleted = hash.hdel(commandElems.subList(2, commandElems.size()));
command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(), numDeleted));
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetSynchronized.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetSynchronized.java
index f667ebe..dd60666 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetSynchronized.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetSynchronized.java
@@ -24,6 +24,7 @@ import java.util.concurrent.atomic.AtomicLong;
import org.apache.geode.cache.Region;
import org.apache.geode.redis.internal.ByteArrayWrapper;
import org.apache.geode.redis.internal.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.RedisDataType;
class GeodeRedisSetSynchronized implements RedisSet {
@@ -59,8 +60,17 @@ class GeodeRedisSetSynchronized implements RedisSet {
Set<ByteArrayWrapper> newValue = createSet(oldValue);
newValue.removeAll(membersToRemove);
removedCount.set(oldValue.size() - newValue.size());
+
return newValue;
});
+
+ if (members().isEmpty()) {
+ RedisDataType type = context.getKeyRegistrar().getType(key);
+ if (type == RedisDataType.REDIS_SET) {
+ context.getRegionProvider().removeKey(key, type);
+ }
+ }
+
return removedCount.longValue();
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SRemExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SRemExecutor.java
index 4901741..9dacef4 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SRemExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SRemExecutor.java
@@ -37,6 +37,7 @@ public class SRemExecutor extends SetExecutor {
checkDataType(key, RedisDataType.REDIS_SET, context);
RedisSet set = new GeodeRedisSetSynchronized(key, context);
long numRemoved = set.srem(commandElems.subList(2, commandElems.size()));
+
command.setResponse(Coder.getIntegerResponse(context.getByteBufAllocator(), numRemoved));
}
}
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/GetSetExecutorJUnitTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/GetSetExecutorJUnitTest.java
index 2599843..16f013a 100644
--- a/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/GetSetExecutorJUnitTest.java
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/executor/string/GetSetExecutorJUnitTest.java
@@ -17,9 +17,7 @@
package org.apache.geode.redis.internal.executor.string;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
-import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;
@@ -39,7 +37,6 @@ import org.apache.geode.redis.internal.ByteArrayWrapper;
import org.apache.geode.redis.internal.Command;
import org.apache.geode.redis.internal.ExecutionHandlerContext;
import org.apache.geode.redis.internal.KeyRegistrar;
-import org.apache.geode.redis.internal.RedisDataTypeMismatchException;
import org.apache.geode.redis.internal.RedisLockService;
import org.apache.geode.redis.internal.RegionProvider;
@@ -99,22 +96,4 @@ public class GetSetExecutorJUnitTest {
assertThat(command.getResponse().toString(Charset.defaultCharset()))
.startsWith("-ERR The wrong number of arguments or syntax was provided");
}
-
- @Test
- public void test_givenKeyHoldsWrongType_returnsError() {
- List<byte[]> args = Arrays.asList(
- "GETSET".getBytes(),
- "key1".getBytes(),
- "val1".getBytes());
- Command command = new Command(args);
-
- when(region.get(any())).thenReturn(new ByteArrayWrapper("non-null value".getBytes()));
- doThrow(new RedisDataTypeMismatchException("this string doesn't matter")).when(executor)
- .checkDataType(any(), any(), any());
-
- executor.executeCommand(command, context);
-
- assertThat(command.getResponse().toString(Charset.defaultCharset()))
- .startsWith("-ERR WRONGTYPE Operation against a key holding the wrong kind of value");
- }
}