dschneider-pivotal commented on a change in pull request #7172:
URL: https://github.com/apache/geode/pull/7172#discussion_r768871085
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +102,34 @@ public RedisSet(int size) {
return diff;
}
+ private static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ Set<byte[]> diff) {
+ if (regionProvider.getRedisData(destinationKey).getType() != REDIS_SET) {
+ regionProvider.getDataRegion().remove(destinationKey);
Review comment:
In this "if" block you can set "destinationKeyExists" to true since you
are doing a remove.
Then add an "else" to this block that does the "exists" call on line 111.
Also instead of calling both exists and getTypedRedisData, change the code
to just call getTypedRedisData. If it returns null that is the same as exists
returning false. We should only need to do one region op to find the entry;
then we modify it, and then do a region op to store the changes.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/ReplaceByteArrays.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.internal.InternalDataSerializer.readSet;
+import static
org.apache.geode.redis.internal.data.delta.DeltaType.REPLACE_BYTE_ARRAYS;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+
+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.writeSet(byteArrays, out);
Review comment:
I think this code would be better if you didn't use writeSet and readSet.
They have to encode in the bytes the type of each set element.
You can instead follow the example of AddByteArrays by writing your set's
length (using writeArrayLength) and then iterating over it and calling
writeByteArray for each element.
On the read side I think it would be better to add them one at a time like
we with AddByteArrays instead of collecting them all into a set. You just need
to expose an extra method (like applyStartReplaceByteArraysDelta) that just
does the clear and then applyReplaceByteArray that just adds one byte[] instead
of a whole set.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/ReplaceByteArrays.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.internal.InternalDataSerializer.readSet;
+import static
org.apache.geode.redis.internal.data.delta.DeltaType.REPLACE_BYTE_ARRAYS;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+
+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.writeSet(byteArrays, out);
Review comment:
Another way to do this in deserializeFrom would be to create an actual
new MemberSet that you will pass to applyReplaceByteArraysDelta. Then you don't
need to clear the old one or update it in any way. You will just change the
"members" instance variable to refer to the new MemberSet.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -224,6 +260,11 @@ public void applyRemoveByteArrayDelta(byte[] bytes) {
membersRemove(bytes);
}
+ @Override
+ public void applyReplaceByteArraysDelta(Set<byte[]> bytes) {
+ members = new MemberSet(bytes);
Review comment:
If you know "bytes" is a MemberSet then no need to create a new
MemberSet here.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +102,34 @@ public RedisSet(int size) {
return diff;
}
+ private static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ Set<byte[]> diff) {
+ if (regionProvider.getRedisData(destinationKey).getType() != REDIS_SET) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+
+ boolean destinationKeyExists =
regionProvider.getRedisData(destinationKey).exists();
+ if (diff.isEmpty()) {
+ if (destinationKeyExists) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+ return 0;
+ }
+
+ if (destinationKeyExists) {
+ RedisSet destinationSet = regionProvider.getTypedRedisData(REDIS_SET,
destinationKey, false);
+
+ destinationSet.members.clear();
+ destinationSet.members.addAll(diff);
+ destinationSet.storeChanges(regionProvider.getDataRegion(),
destinationKey,
+ new ReplaceByteArrays(diff));
+ } else {
+ regionProvider.getDataRegion().put(destinationKey, new RedisSet(diff));
Review comment:
If you know "diff" is a MemberSet then you can add a RedisSet
constructor for it that just uses diff instead of creating a new MemberSet.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -241,6 +244,9 @@ public void fromDelta(DataInput in) throws IOException,
InvalidDeltaException {
case SET_BYTE_ARRAY_AND_TIMESTAMP:
SetByteArrayAndTimestamp.deserializeFrom(in, this);
break;
+ case REPLACE_BYTE_ARRAYS:
+ ReplaceByteArrays.deserializeFrom(in, this);
Review comment:
this call should be synchronized (like ADD_BYTE_ARRAYS).
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -224,6 +260,11 @@ public void applyRemoveByteArrayDelta(byte[] bytes) {
membersRemove(bytes);
}
+ @Override
+ public void applyReplaceByteArraysDelta(Set<byte[]> bytes) {
+ members = new MemberSet(bytes);
Review comment:
something missing here is a call to persistNoDelta
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +102,34 @@ public RedisSet(int size) {
return diff;
}
+ private static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ Set<byte[]> diff) {
+ if (regionProvider.getRedisData(destinationKey).getType() != REDIS_SET) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+
+ boolean destinationKeyExists =
regionProvider.getRedisData(destinationKey).exists();
+ if (diff.isEmpty()) {
+ if (destinationKeyExists) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+ return 0;
+ }
+
+ if (destinationKeyExists) {
+ RedisSet destinationSet = regionProvider.getTypedRedisData(REDIS_SET,
destinationKey, false);
+
+ destinationSet.members.clear();
Review comment:
If you know "diff" is a MemberSet then instead of clear and addAll you
can just do "destinationSet.members = diff".
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -94,6 +102,34 @@ public RedisSet(int size) {
return diff;
}
+ private static int setOpStoreResult(RegionProvider regionProvider, RedisKey
destinationKey,
+ Set<byte[]> diff) {
+ if (regionProvider.getRedisData(destinationKey).getType() != REDIS_SET) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+
+ boolean destinationKeyExists =
regionProvider.getRedisData(destinationKey).exists();
+ if (diff.isEmpty()) {
+ if (destinationKeyExists) {
+ regionProvider.getDataRegion().remove(destinationKey);
+ }
+ return 0;
+ }
+
+ if (destinationKeyExists) {
+ RedisSet destinationSet = regionProvider.getTypedRedisData(REDIS_SET,
destinationKey, false);
+
+ destinationSet.members.clear();
Review comment:
something missing here is a call to persistNoDelta
--
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]
