You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/01 09:25:42 UTC
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
jdeppe-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699347453
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZaddsDeltaInfo.java
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.delta;
+
+import static org.apache.geode.redis.internal.delta.DeltaType.ZADDS;
+
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+
+public class ZaddsDeltaInfo implements DeltaInfo {
+ private final ArrayList<byte[]> deltas = new ArrayList<>();
+ private final ArrayList<Double> scores = new ArrayList<>();
+
+ public ZaddsDeltaInfo() {}
+
+ public ZaddsDeltaInfo(List<byte[]> deltas, List<Double> scores) {
+ this.deltas.addAll(deltas);
+ this.scores.addAll(scores);
+ }
+
+ public ZaddsDeltaInfo(byte[] delta, Double score) {
+ this.deltas.add(delta);
+ this.scores.add(score);
+ }
+
+ public void add(byte[] delta, double score) {
+ deltas.add(delta);
+ this.scores.add(score);
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ InternalDataSerializer.writeEnum(ZADDS, out);
+ InternalDataSerializer.writeArrayLength(deltas.size(), out);
+ for (int i = 0; i < deltas.size(); i++) {
+ DataSerializer.writeByteArray(deltas.get(i), out);
+ DataSerializer.writeDouble(scores.get(i), out);
+ }
+ }
+
+ public List<byte[]> getZAddsMembers() {
+ return deltas;
+ }
+
+ public List<Double> getZAddScores() {
+ return scores;
+ }
Review comment:
Even more nitty, but for consistency could these methods use the same prefix - either `getZAdd` or `getZAdds`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZaddsDeltaInfo.java
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.delta;
+
+import static org.apache.geode.redis.internal.delta.DeltaType.ZADDS;
+
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+
+public class ZaddsDeltaInfo implements DeltaInfo {
Review comment:
Sorry, this feels slightly nitty, but could you make this `ZAddsDelta...` in keeping with the format of the other `DeltaInfo` classes.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZaddsDeltaInfo.java
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.delta;
+
+import static org.apache.geode.redis.internal.delta.DeltaType.ZADDS;
+
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+
+public class ZaddsDeltaInfo implements DeltaInfo {
+ private final ArrayList<byte[]> deltas = new ArrayList<>();
+ private final ArrayList<Double> scores = new ArrayList<>();
+
+ public ZaddsDeltaInfo() {}
+
+ public ZaddsDeltaInfo(List<byte[]> deltas, List<Double> scores) {
+ this.deltas.addAll(deltas);
+ this.scores.addAll(scores);
+ }
+
+ public ZaddsDeltaInfo(byte[] delta, Double score) {
+ this.deltas.add(delta);
+ this.scores.add(score);
+ }
+
+ public void add(byte[] delta, double score) {
+ deltas.add(delta);
+ this.scores.add(score);
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ InternalDataSerializer.writeEnum(ZADDS, out);
+ InternalDataSerializer.writeArrayLength(deltas.size(), out);
Review comment:
We should just be using `DataSerializer` and not the internal version. Instead of `writeArrayLength` you can do `DataSerializer.writePrimitiveInt(deltas.size(), out)`
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -404,9 +404,14 @@ public static double bytesToDouble(byte[] bytes) {
return NEGATIVE_INFINITY;
}
if (isNaN(bytes)) {
- return NaN;
+ throw new NumberFormatException();
Review comment:
Since `Double.NaN` is a thing, it seems inconsistent that we're able to return `*INFINITY`s but throw an exception with `NaN`. Can this exception be thrown, at a higher level where `NaN` is determined to be invalid?
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -265,30 +292,27 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
return maxIndex - minIndex;
}
- byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
- byte[] member) {
+ double zincrby(Region<RedisKey, RedisData> region, RedisKey key, double incr, byte[] member) {
+ double score;
OrderedSetEntry orderedSetEntry = members.get(member);
- double incr = processByteArrayAsDouble(increment);
if (orderedSetEntry != null) {
- double byteScore = orderedSetEntry.getScore();
- incr += byteScore;
+ score = orderedSetEntry.getScore() + incr;
- if (Double.isNaN(incr)) {
+ if (Double.isNaN(score)) {
Review comment:
Since we're not converting a byte array into a double anymore, I'd prefer to see this check removed. In general this type of validation should be occurring at a higher level - in the `Executor`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -213,34 +232,42 @@ private synchronized void membersRemoveAll(RemsDeltaInfo remsDeltaInfo) {
* @return the number of members actually added OR incremented value if INCR option specified
*/
Object zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]> membersToAdd,
- ZAddOptions options) {
+ List<Double> scoresToAdd, ZAddOptions options) {
if (options.isINCR()) {
- return zaddIncr(region, key, membersToAdd, options);
+ // if INCR, only one score and member can be added
+ return zaddIncr(region, key, membersToAdd.get(0), scoresToAdd.get(0), options);
+ }
+
+ if (membersToAdd.size() != scoresToAdd.size()) {
+ throw new NumberFormatException("there must be the same number of members and scores");
}
- AddsDeltaInfo deltaInfo = null;
- Iterator<byte[]> iterator = membersToAdd.iterator();
+
+ ZaddsDeltaInfo deltaInfo = null;
+ Iterator<byte[]> membersIterator = membersToAdd.iterator();
+ Iterator<Double> scoresIterator = scoresToAdd.iterator();
+
int initialSize = scoreSet.size();
int changesCount = 0;
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+
+ while (membersIterator.hasNext()) {
Review comment:
I think it's more idiomatic to just use a `for(...)` loop here. Previously we had an iterator because members and scores were interleaved in a single list.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -67,14 +65,19 @@ public int getSizeInBytes() {
return REDIS_SORTED_SET_OVERHEAD + members.getSizeInBytes() + scoreSet.getSizeInBytes();
}
- RedisSortedSet(List<byte[]> members) {
- this.members = new MemberMap(members.size() / 2);
+ RedisSortedSet(List<byte[]> members, List<Double> scores) {
+ this.members = new MemberMap(members.size() + scores.size());
- Iterator<byte[]> iterator = members.iterator();
+ if (members.size() != scores.size()) {
+ throw new NumberFormatException("there must be the same number of members and scores");
Review comment:
I think `IllegalArgumentException` is more appropriate here.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -200,6 +203,15 @@ public void fromDelta(DataInput in) throws IOException, InvalidDeltaException {
byte[] byteArray = DataSerializer.readByteArray(in);
applyDelta(new AppendDeltaInfo(byteArray, sequence));
break;
+ case ZADDS:
+ int numMembers = InternalDataSerializer.readArrayLength(in);
+ List<byte[]> members = new ArrayList<>();
+ List<Double> scores = new ArrayList<>();
Review comment:
Use `DataSerializer.readPrimitiveInt` (see more comments from `ZAddsDeltaInfo`). Also, this value can then be used to initialize the 2 arrays.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -213,34 +232,42 @@ private synchronized void membersRemoveAll(RemsDeltaInfo remsDeltaInfo) {
* @return the number of members actually added OR incremented value if INCR option specified
*/
Object zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]> membersToAdd,
- ZAddOptions options) {
+ List<Double> scoresToAdd, ZAddOptions options) {
if (options.isINCR()) {
- return zaddIncr(region, key, membersToAdd, options);
+ // if INCR, only one score and member can be added
+ return zaddIncr(region, key, membersToAdd.get(0), scoresToAdd.get(0), options);
+ }
+
+ if (membersToAdd.size() != scoresToAdd.size()) {
+ throw new NumberFormatException("there must be the same number of members and scores");
Review comment:
I think IllegalArgumentException is more appropriate here.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZaddsDeltaInfo.java
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.delta;
+
+import static org.apache.geode.redis.internal.delta.DeltaType.ZADDS;
+
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+
+public class ZaddsDeltaInfo implements DeltaInfo {
+ private final ArrayList<byte[]> deltas = new ArrayList<>();
+ private final ArrayList<Double> scores = new ArrayList<>();
+
+ public ZaddsDeltaInfo() {}
+
+ public ZaddsDeltaInfo(List<byte[]> deltas, List<Double> scores) {
+ this.deltas.addAll(deltas);
+ this.scores.addAll(scores);
+ }
Review comment:
We should just use the references passed in here instead of copying everything again into our internal arrays.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/SortedSetScoreRangeOptions.java
##########
@@ -48,16 +48,10 @@ void parseRangeArguments(List<byte[]> commandElements) {
score = 0.0;
} else {
score = bytesToDouble(Arrays.copyOfRange(bytes, 1, bytes.length));
- if (Double.isNaN(score)) {
- throw new NumberFormatException();
- }
}
return new RangeLimit<>(score, true);
} else {
score = bytesToDouble(bytes);
- if (Double.isNaN(score)) {
Review comment:
Ditto the above
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/SortedSetScoreRangeOptions.java
##########
@@ -48,16 +48,10 @@ void parseRangeArguments(List<byte[]> commandElements) {
score = 0.0;
} else {
score = bytesToDouble(Arrays.copyOfRange(bytes, 1, bytes.length));
- if (Double.isNaN(score)) {
- throw new NumberFormatException();
- }
Review comment:
If you apply my comments in `Coder` this probably needs to be reverted.
--
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