dschneider-pivotal commented on a change in pull request #7172:
URL: https://github.com/apache/geode/pull/7172#discussion_r771013096
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -70,30 +71,67 @@ public RedisSet(int expectedSize) {
public RedisSet() {}
public static Set<byte[]> sdiff(RegionProvider regionProvider,
List<RedisKey> keys) {
- return calculateDiff(regionProvider, keys);
+ MemberSet result = calculateDiff(regionProvider, keys, true);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
}
- private static Set<byte[]> calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys) {
- RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), true);
+ public static int sdiffstore(RegionProvider regionProvider, RedisKey
destinationKey,
+ List<RedisKey> keys) {
+ MemberSet diff = calculateDiff(regionProvider, keys, false);
+ return setOpStoreResult(regionProvider, destinationKey, diff);
+ }
+
+ private static MemberSet calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys,
+ boolean updateStats) {
+ RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), updateStats);
if (firstSet.scard() == 0) {
- return Collections.emptySet();
+ return null;
}
- Set<byte[]> diff = new MemberSet(firstSet.members);
+ MemberSet diff = new MemberSet(firstSet.members);
for (int i = 1; i < keys.size(); i++) {
- RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(i), true);
+ RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(i), updateStats);
if (curSet.scard() == 0) {
continue;
}
diff.removeAll(curSet.members);
if (diff.isEmpty()) {
- return Collections.emptySet();
+ return null;
}
}
return diff;
}
+ @VisibleForTesting
+ static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ MemberSet diff) {
+ RedisSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SET, destinationKey,
false);
+
+ if (diff == null) {
+ if (destinationSet != null) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+ return 0;
+ }
+
+ if (destinationSet != null) {
+ destinationSet.persistNoDelta();
+ destinationSet.members.clear();
Review comment:
no need to call "clear" any longer since the next line sets "members" to
diff.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -70,30 +71,67 @@ public RedisSet(int expectedSize) {
public RedisSet() {}
public static Set<byte[]> sdiff(RegionProvider regionProvider,
List<RedisKey> keys) {
- return calculateDiff(regionProvider, keys);
+ MemberSet result = calculateDiff(regionProvider, keys, true);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
}
- private static Set<byte[]> calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys) {
- RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), true);
+ public static int sdiffstore(RegionProvider regionProvider, RedisKey
destinationKey,
+ List<RedisKey> keys) {
+ MemberSet diff = calculateDiff(regionProvider, keys, false);
+ return setOpStoreResult(regionProvider, destinationKey, diff);
+ }
+
+ private static MemberSet calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys,
+ boolean updateStats) {
+ RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), updateStats);
if (firstSet.scard() == 0) {
- return Collections.emptySet();
+ return null;
}
- Set<byte[]> diff = new MemberSet(firstSet.members);
+ MemberSet diff = new MemberSet(firstSet.members);
for (int i = 1; i < keys.size(); i++) {
- RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(i), true);
+ RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(i), updateStats);
if (curSet.scard() == 0) {
continue;
}
diff.removeAll(curSet.members);
if (diff.isEmpty()) {
- return Collections.emptySet();
+ return null;
}
}
return diff;
}
+ @VisibleForTesting
+ static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ MemberSet diff) {
+ RedisSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SET, destinationKey,
false);
+
+ if (diff == null) {
+ if (destinationSet != null) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+ return 0;
+ }
+
+ if (destinationSet != null) {
+ destinationSet.persistNoDelta();
+ destinationSet.members.clear();
+ destinationSet.members = diff;
+ destinationSet.storeChanges(regionProvider.getDataRegion(),
destinationKey,
+ new ReplaceByteArrays(diff));
+ } else {
+ regionProvider.getDataRegion().put(destinationKey, new RedisSet(diff));
Review comment:
since "diff" is now a MemberSet and it can be used as the members value
of the RedisSet being constructed; create a new constructor on RedisSet that
takes a MemberSet. It can just set "members" to diff.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/ReplaceByteArrays.java
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.data.delta;
+
+import static org.apache.geode.DataSerializer.readByteArray;
+import static org.apache.geode.internal.InternalDataSerializer.readArrayLength;
+import static
org.apache.geode.redis.internal.data.delta.DeltaType.REPLACE_BYTE_ARRAYS;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.Set;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.redis.internal.data.AbstractRedisData;
+
+public class ReplaceByteArrays implements DeltaInfo {
+ private final Set<byte[]> byteArrays;
+
+ public ReplaceByteArrays(Set<byte[]> deltas) {
+ this.byteArrays = deltas;
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ DataSerializer.writeEnum(REPLACE_BYTE_ARRAYS, out);
+ InternalDataSerializer.writeArrayLength(byteArrays.size(), out);
+ for (byte[] bytes : byteArrays) {
+ DataSerializer.writeByteArray(bytes, out);
+ }
+ }
+
+ // Create a member set as you read it in
+ public static void deserializeFrom(DataInput in, AbstractRedisData
redisData) throws IOException {
+ redisData.applyReplaceByteArraysDelta();
Review comment:
I think what would be better here is to have applyReplaceByteArraysDelta
take a MemberSet. This method will create that MemberSet (call it members) with
the size it reads on the next line. Then in the while loop it will just add
each byte array it reads to that MemberSet. The last thing it does after the
while loop is done is call redisData.applyReplaceByteArraysDelta(members);
All applyReplaceByteArraysDelta does is call persistNoDelta() and
this.members = members.
Sorry that this was confusing. I know I talked about doing it this way and
the way you did it. I think doing it with a MemberSet will be the most
efficient and is consistent with how you did it on the primary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
