[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6877: GEODE-8840: Support RENAMENX command

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #6877:
URL: https://github.com/apache/geode/pull/6877#discussion_r716856855



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -135,12 +137,22 @@ public void restore(RedisKey key, long ttl, byte[] data, RestoreOptions options)
   }
 
   @Override
-  public boolean rename(RedisKey oldKey, RedisKey newKey) {
+  public boolean rename(RedisKey oldKey, RedisKey newKey, boolean ifTargetNotExists) {
     List<RedisKey> lockOrdering = Arrays.asList(oldKey, newKey);
 
     return stripedExecute(oldKey, lockOrdering,
-        () -> getRedisData(oldKey).rename(getRegionProvider().getLocalDataRegion(), oldKey,
-            newKey));
+        () -> {
+          RedisData sourceData = getRedisData(oldKey);
+          // nonexistent source takes priority over nonexistent target
+          if (!sourceData.exists()) {
+            return false;
+          }
+          if (ifTargetNotExists && getRedisData(newKey).exists()) {

Review comment:
       I think it would be better if you pushed this logic down into AbstractRedisData.rename by passing it "ifTargetNotExists". Down at that level you can use putIfAbsent or create on the region instead of doing an extra get up at this level. This will also allow you to get rid of the check of sourceData.exists() at this level since you get that for free from the NullRedisData implementation.
   I also prefer we keep these *FunctionExecutor classes focused on locking and striped execution.




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org