ringles commented on a change in pull request #5146:
URL: https://github.com/apache/geode/pull/5146#discussion_r429278269
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -205,281 +98,27 @@ private boolean
typeStoresDataInKeyRegistrar(RedisDataType type) {
}
public boolean removeKey(ByteArrayWrapper key, RedisDataType type, boolean
cancelExpiration) {
- if (type == RedisDataType.REDIS_PROTECTED) {
- return false;
+ if (!typeStoresDataInKeyRegistrar(type)) {
+ keyRegistrar.unregister(key);
}
- Lock lock = dynamicRegionLocks.get(key);
+ RedisKeyCommands redisKeyCommands = new
RedisKeyCommandsFunctionExecutor(dataRegion);
try {
- if (lock != null) { // only typeUsesDynamicRegions will have a lock
- lock.lock();
- }
- if (!typeStoresDataInKeyRegistrar(type)) {
- keyRegistrar.unregister(key);
- }
- RedisKeyCommands redisKeyCommands = new
RedisKeyCommandsFunctionExecutor(dataRegion);
- try {
- if (type == RedisDataType.REDIS_STRING) {
- return stringsRegion.remove(key) != null;
- } else if (type == RedisDataType.REDIS_HLL) {
- return hLLRegion.remove(key) != null;
- } else if (type == RedisDataType.REDIS_LIST || type ==
RedisDataType.REDIS_SORTEDSET) {
- return destroyRegion(key, type);
- } else if (type == RedisDataType.REDIS_SET || type ==
RedisDataType.REDIS_HASH) {
- return redisKeyCommands.del(key);
- } else {
- return false;
- }
- } catch (Exception exc) {
+ if (type == RedisDataType.REDIS_STRING) {
+ return stringsRegion.remove(key) != null;
+ } else if (type == RedisDataType.REDIS_SET || type ==
RedisDataType.REDIS_HASH) {
+ return redisKeyCommands.del(key);
+ } else {
return false;
- } finally {
- if (cancelExpiration) {
- cancelKeyExpiration(key);
- } else {
- removeKeyExpiration(key);
- }
- if (lock != null) {
- dynamicRegionLocks.remove(key);
- }
}
+ } catch (Exception exc) {
+ return false;
} finally {
- if (lock != null) {
- lock.unlock();
- }
- }
- }
-
- public Region<?, ?> getOrCreateRegion(ByteArrayWrapper key, RedisDataType
type,
- ExecutionHandlerContext context) {
- return getOrCreateRegion0(key, type, context, true);
- }
-
- public boolean typeUsesDynamicRegions(RedisDataType type) {
- return type == RedisDataType.REDIS_LIST || type ==
RedisDataType.REDIS_SORTEDSET;
- }
-
- public void createRemoteRegionReferenceLocally(ByteArrayWrapper key,
RedisDataType type) {
- if (!typeUsesDynamicRegions(type)) {
- return;
- }
- Region<Object, Object> r = dynamicRegions.get(key);
- if (r != null) {
- return;
- }
- Lock lock = dynamicRegionLocks.get(key);
- if (lock == null) {
- Lock newLock = new ReentrantLock();
- lock = dynamicRegionLocks.putIfAbsent(key, newLock);
- if (lock == null) {
- lock = newLock;
- }
- }
- boolean locked = lock.tryLock();
- // If we cannot get the lock then this remote event may have been
initialized
- // independently on this machine, so if we wait on the lock it is more than
- // likely we will deadlock just to do the same task. This event can be
ignored
- if (locked) {
- try {
- r = cache.getRegion(key.toString());
- // If r is null, this implies that we are after a create/destroy
- // simply ignore. Calls to getRegion or getOrCreate will work correctly
- if (r == null) {
- // TODO: one caller of this method only calls it if getRegion
returned null. It was
- // expecting us to create it locally. If someone else will create it
locally then this
- // method does not need to be called.
- return;
- }
-
- if (type == RedisDataType.REDIS_LIST) {
- doInitializeList(key, r);
- } else if (type == RedisDataType.REDIS_SORTEDSET) {
- try {
- doInitializeSortedSet(key, r);
- } catch (RegionNotFoundException | IndexInvalidException e) {
- // ignore
- }
- }
- dynamicRegions.put(key, r);
- } finally {
- lock.unlock();
- }
- }
- }
-
- private Region<?, ?> getOrCreateRegion0(ByteArrayWrapper key, RedisDataType
type,
- ExecutionHandlerContext context, boolean addToMeta) {
-
- keyRegistrar.validate(key, type);
- Region<Object, Object> r = dynamicRegions.get(key);
- if (r != null && r.isDestroyed()) {
- removeKey(key, type);
- r = null;
- }
- if (r == null) {
- Lock lock = dynamicRegionLocks.get(key);
- if (lock == null) {
- Lock newLock = new ReentrantLock();
- lock = dynamicRegionLocks.putIfAbsent(key, newLock);
- if (lock == null) {
- lock = newLock;
- }
- }
-
- lock.lock();
- try {
- r = dynamicRegions.get(key);
- if (r == null) {
- boolean hasTransaction = context != null &&
context.hasTransaction(); // Can create
- // without context
- CacheTransactionManager txm = null;
- TransactionId transactionId = null;
- try {
- if (hasTransaction) {
- txm = cache.getCacheTransactionManager();
- transactionId = txm.suspend();
- }
- Exception concurrentCreateDestroyException;
- do {
- concurrentCreateDestroyException = null;
-
- r = createRegionGlobally(key.toString());
-
- try {
- if (type == RedisDataType.REDIS_LIST) {
- doInitializeList(key, r);
- } else if (type == RedisDataType.REDIS_SORTEDSET) {
- try {
- doInitializeSortedSet(key, r);
- } catch (RegionNotFoundException | IndexInvalidException e) {
- concurrentCreateDestroyException = e;
- }
- }
- } catch (QueryInvalidException e) {
- if (e.getCause() instanceof RegionNotFoundException) {
- concurrentCreateDestroyException = e;
- }
- }
- } while (concurrentCreateDestroyException != null);
- dynamicRegions.put(key, r);
- if (addToMeta) {
- keyRegistrar.register(key, type);
- }
- } finally {
- if (hasTransaction) {
- txm.resume(transactionId);
- }
- }
- }
- } finally {
- lock.unlock();
- }
- }
- return r;
- }
-
- /**
- * SYNCHRONIZE EXTERNALLY OF this.locks.get(key)!!!!!
- *
- * @param key Key of region to destroy
- * @param type Type of region to destroyu
- * @return Flag if destroyed
- */
- private boolean destroyRegion(ByteArrayWrapper key, RedisDataType type) {
- Region<?, ?> r = dynamicRegions.get(key);
- if (r != null) {
- try {
- r.destroyRegion();
- } catch (Exception e) {
- return false;
- } finally {
- removeRegionState(key, type);
+ if (cancelExpiration) {
Review comment:
So, because we don't care whether or not these calls return true or
false, we don't really need the lock anymore? If two threads try to remove the
same key, at least one will succeed so no harm, no foul?
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java
##########
@@ -201,11 +172,7 @@ private void throwDataTypeException(ByteArrayWrapper key,
RedisData data) {
}
private void throwDataTypeException(ByteArrayWrapper key, RedisDataType
dataType) {
- if (RedisDataType.REDIS_PROTECTED.equals(dataType)) {
- throw new RedisDataTypeMismatchException("The key name \"" + key + "\"
is protected");
- } else {
- throw new RedisDataTypeMismatchException(
- RedisConstants.ERROR_WRONG_TYPE);
- }
+ throw new RedisDataTypeMismatchException(
Review comment:
Doesn't look like the arguments are needed anymore. In fact, the whole
function may be eliminated...
##########
File path:
geode-redis/src/distributedTest/java/org/apache/geode/redis/RedisDistDUnitTest.java
##########
@@ -209,18 +209,6 @@ public void run() throws InterruptedException {
} else {
jedis.del(hKey);
}
- } else if (n == 1) {
Review comment:
Can we get rid of the @Ignore on this test now that we're only testing
hashes and sets?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
