[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5152: Change Redis Rename Functions to Make use of Striped Executor

[GitBox]

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



##########
File path: 
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisKeyInRegion.java
##########
@@ -46,4 +47,13 @@ public boolean del(ByteArrayWrapper key) {
   public boolean exists(ByteArrayWrapper key) {
     return localRegion.containsKey(key);
   }
+
+  public boolean rename(ByteArrayWrapper oldKey, ByteArrayWrapper newKey) {
+
+    Object value = localRegion.get(oldKey);

Review comment:
       It looks like this impl will not work on String since they do not yet 
store their data in the region.
   It might be best just to wait for the String work to complete

##########
File path: 
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisKeyInRegion.java
##########
@@ -46,4 +47,13 @@ public boolean del(ByteArrayWrapper key) {
   public boolean exists(ByteArrayWrapper key) {
     return localRegion.containsKey(key);
   }
+
+  public boolean rename(ByteArrayWrapper oldKey, ByteArrayWrapper newKey) {
+
+    Object value = localRegion.get(oldKey);

Review comment:
       What if get returns null? I think in that case rename should immediately 
return false.




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