[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7172: feature/GEODE-9828: support SDIFFSTORE command

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

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



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +101,16 @@ public RedisSet(int size) {
     return diff;
   }
 
+  private static int setOpStoreResult(RegionProvider regionProvider, RedisKey destinationKey,
+      Set<byte[]> diff) {
+    if (diff.isEmpty()) {
+      regionProvider.getDataRegion().remove(destinationKey);
+    } else {
+      regionProvider.getLocalDataRegion().put(destinationKey, new RedisSet(diff));

Review comment:
       I'm pretty sure we don't need to be using a different region accessor method for remove vs. put. Both of these are write ops so I believe getLocalDataRegion has make no difference (it returns PartitionRegionHelper.getLocalPrimaryData which only allows local reads ops but the write ops can be local or remote).
   Since getting getLocalPrimaryData when should probably prefer to use getDataRegion() when doing write ops.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +101,16 @@ public RedisSet(int size) {
     return diff;
   }
 
+  private static int setOpStoreResult(RegionProvider regionProvider, RedisKey destinationKey,
+      Set<byte[]> diff) {
+    if (diff.isEmpty()) {
+      regionProvider.getDataRegion().remove(destinationKey);
+    } else {
+      regionProvider.getLocalDataRegion().put(destinationKey, new RedisSet(diff));

Review comment:
       for the "region.put" case you should do a check to see if destinationKey exists. If it doesn't do the put. But otherwise modify the existing RedisSet in place and call storeChanges with a DeltaInfo. You will want to merge in the latest develop to get the fix for GEODE-9772. The new DeltaType could be SET_BYTE_ARRAYS and it would just be a List of byte[] like like ADD_BYTE_ARRAYS. The difference is that when this DeltaInfo is applied you will want to first call something on the RedisSet that clears it and calls persistNoDelta and then just calls applyAddByteArrayDelta repeatedly. You will also need to add a method on the near side (not the delta apply side) that does this (given a Set<byte[]> does persistNoDelta() and "members = new MemberSet(diff)". Even better would be to make sure calculateDIff always returns a MemberSet instead of a Set and then this could just be "member = diff". I think the only case when it is not a MemberSet is when it is empty in which case you do a re
 gion.remove so in this code you could just cast diff to MemberSet safely.
   I can help with this delta work if you have any questions.
   




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