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/08/20 22:46:38 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #6783: GEODE-9446 remove unecessary byte arrays
nonbinaryprogrammer opened a new pull request #6783:
URL: https://github.com/apache/geode/pull/6783
RedisSortedSet previously stored the score as both a double and a byte
array in order to cut down on conversions using Coder. This refactor
gets rid of the byte array score and instead does a couple of
conversions. Other refactors were done to accomodate this change and
to improve on what was there
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r702155507
##########
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()) {
+ double score = scoresIterator.next();
+ byte[] member = membersIterator.next();
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
if (deltaInfo == null) {
- deltaInfo = new AddsDeltaInfo(new ArrayList<>());
+ deltaInfo = new ZaddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
Review comment:
I realize now that this suggestion is incorrect, because we actually have three possible cases: a new entry was added, an existing entry was updated or an existing entry was not updated. We need to know if the second case is true when the `CH` option is selected, but if the third case is true, then we don't need to send a delta or update the region. It would be good if we could find a way to cover both cases, since updating the region for no-ops seems wasteful.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r707581828
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +52,18 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())),
+ int size = (commandElements.size() - optionsFoundCount - 2) / 2;
+ List<byte[]> members = new ArrayList<>(size);
+ List<Double> scores = new ArrayList<>(size);
Review comment:
sorry, guess I didn't change all of them
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r707608915
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -262,30 +258,26 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
return maxIndex - minIndex;
}
- byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
- byte[] member) {
+ byte[] 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)) {
throw new NumberFormatException(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
}
+ } else {
+ score = incr;
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
-
- AddsDeltaInfo deltaInfo = new AddsDeltaInfo(new ArrayList<>());
- deltaInfo.add(member);
- deltaInfo.add(byteIncr);
-
- storeChanges(region, key, deltaInfo);
+ if (!(memberAdd(member, score) == MemberAddResult.NO_OP)) {
+ ZAddsDeltaInfo deltaInfo = new ZAddsDeltaInfo(1);
+ storeChanges(region, key, deltaInfo);
Review comment:
thanks
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r706334841
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZAddsDeltaInfo.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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;
+
+public class ZAddsDeltaInfo implements DeltaInfo {
Review comment:
To make ZAddsDeltaInfo produce less garbage consider changing it to have a "double[] scores" instead of "List<Double> scores". Every time we have a "double" and need to convert it to Double an object allocation happens.
Change it to have a single constructor "ZAddsDeltaInfo(int size)". The usage pattern would be to create it with the max size and then just call "add" for each pair you want to add. It looked to me like both places we create a ZAddsDeltaInfo we already know its max size. The array of doubles may not get completely filled but as long as you use the "deltas" List's size() to determine how many things are in the array you will be good. You might even want to add a size() method to this class that returns deltas.size().
After making these changes make sure you don't have any places that use Double. They should all use "double". One place I noticed was when you call DataSerializer.writeDouble. You will want to change it to: DataSerializer.writePrimitiveDouble.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +52,18 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())),
+ int size = (commandElements.size() - optionsFoundCount - 2) / 2;
+ List<byte[]> members = new ArrayList<>(size);
+ List<Double> scores = new ArrayList<>(size);
Review comment:
You can change this List<Double> to be "double[]"
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -200,6 +202,15 @@ public void fromDelta(DataInput in) throws IOException, InvalidDeltaException {
byte[] byteArray = DataSerializer.readByteArray(in);
applyDelta(new AppendDeltaInfo(byteArray, sequence));
break;
+ case ZADDS:
+ int numMembers = DataSerializer.readPrimitiveInt(in);
Review comment:
If you make the changes I suggested to ZAddsDeltaInfo this could would look like this:
int numMembers = DataSerializer.readPrimitiveInt(in);
ZAddsDeltaInfo delta = new ZAddsDeltaInfo(numMembers);
for (int i = 0; i < numMembers; i++) {
byte[] member = DataSerializer.readByteArray(in);
double score = DataSerializer.readPrimitiveDouble(in);
delta.add(member, score);
}
applyDelta(delta);
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r706397533
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -200,6 +202,15 @@ public void fromDelta(DataInput in) throws IOException, InvalidDeltaException {
byte[] byteArray = DataSerializer.readByteArray(in);
applyDelta(new AppendDeltaInfo(byteArray, sequence));
break;
+ case ZADDS:
+ int numMembers = DataSerializer.readPrimitiveInt(in);
Review comment:
done
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r706504228
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -262,30 +258,26 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
return maxIndex - minIndex;
}
- byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
- byte[] member) {
+ byte[] 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)) {
throw new NumberFormatException(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
}
+ } else {
+ score = incr;
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
-
- AddsDeltaInfo deltaInfo = new AddsDeltaInfo(new ArrayList<>());
- deltaInfo.add(member);
- deltaInfo.add(byteIncr);
-
- storeChanges(region, key, deltaInfo);
+ if (!(memberAdd(member, score) == MemberAddResult.NO_OP)) {
+ ZAddsDeltaInfo deltaInfo = new ZAddsDeltaInfo(1);
+ storeChanges(region, key, deltaInfo);
Review comment:
The failing DUnit test can be fixed by making this
```
ZAddsDeltaInfo deltaInfo = new ZAddsDeltaInfo(1);
deltaInfo.add(member, score);
storeChanges(region, key, deltaInfo);
```
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699433762
##########
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:
I did that to make the fields final. plus even if we just do `this.deltas = deltas`, we still have to convert deltas to an arraylist
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r706397157
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -105,16 +110,6 @@ public void narrowLongToInt_correctlyConvertsLongToInt() {
}
}
- @Test
- @Parameters(method = "doubleBytes")
- public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input,
Review comment:
added
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699652107
##########
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:
we can check in the executor if the increment is NaN and exit early, but that check in RedisSortedSet is still required. it handles the case where the score is -Infinity and the increment is +Infinity, which are both valid, but adding them produces NaN
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r697717326
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -274,9 +277,9 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
+ memberAdd(member, incr);
Review comment:
I think I addressed this, but let me know if I didn't.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r706397400
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZAddsDeltaInfo.java
##########
@@ -0,0 +1,68 @@
+/*
+ * 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;
+
+public class ZAddsDeltaInfo implements DeltaInfo {
Review comment:
done
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +52,18 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())),
+ int size = (commandElements.size() - optionsFoundCount - 2) / 2;
+ List<byte[]> members = new ArrayList<>(size);
+ List<Double> scores = new ArrayList<>(size);
Review comment:
done
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699652107
##########
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:
we can check in the executor if the increment is NaN and exit early, but that check in RedisSortedSet is still required. it handles the case where the score is -Infinity and the increment is +Infinity, which are both valid, but adding them produces NaN
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699681480
##########
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());
Review comment:
The initial size of `members` should be just `members.size()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -91,6 +94,9 @@ protected void applyDelta(DeltaInfo deltaInfo) {
if (deltaInfo instanceof AddsDeltaInfo) {
AddsDeltaInfo addsDeltaInfo = (AddsDeltaInfo) deltaInfo;
membersAddAll(addsDeltaInfo);
Review comment:
With the changes to this class, `AddsDeltaInfo` should no longer ever be used in this class, as it assumes an interleaved list of member names and scores, which is no longer valid. This means that this if statement and the `membersAddAll()` method can be removed.
##########
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()) {
Review comment:
This check is already made in the Executors that might call this constructor. `ZAddExecutor` and `ZIncryByExecutor` are the only two, I think, and both have logic in place to deal with member/score mismatches, so this check, and the similar one in `zadd()` can be safely removed.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -163,43 +169,56 @@ public int getDSFID() {
return REDIS_SORTED_SET_ID;
}
- protected synchronized byte[] memberAdd(byte[] memberToAdd, byte[] scoreToAdd) {
- OrderedSetEntry existingEntry = members.get(memberToAdd);
- if (existingEntry == null) {
+ protected synchronized boolean memberAdd(byte[] memberToAdd, double scoreToAdd) {
+ OrderedSetEntry entry = members.get(memberToAdd);
+ if (entry == null) {
OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
- return null;
+ return false;
} else {
- scoreSet.remove(existingEntry);
- byte[] oldScore = existingEntry.scoreBytes;
- existingEntry.updateScore(stripTrailingZeroFromDouble(scoreToAdd));
- members.put(memberToAdd, existingEntry);
- scoreSet.add(existingEntry);
- return oldScore;
+ if (entry.score == scoreToAdd) {
+ return false;
+ }
+ updateScore(memberToAdd, scoreToAdd, entry);
}
+ return true;
}
- synchronized byte[] memberRemove(byte[] member) {
- byte[] oldValue = null;
+ private void updateScore(byte[] memberToAdd, double scoreToAdd, OrderedSetEntry entry) {
+ scoreSet.remove(entry);
+ entry.updateScore(scoreToAdd);
+ members.put(memberToAdd, entry);
+ scoreSet.add(entry);
+ }
+
+ synchronized boolean memberRemove(byte[] member) {
OrderedSetEntry orderedSetEntry = members.remove(member);
if (orderedSetEntry != null) {
scoreSet.remove(orderedSetEntry);
- oldValue = orderedSetEntry.getScoreBytes();
+ return true;
}
- return oldValue;
+ return false;
}
private synchronized void membersAddAll(AddsDeltaInfo addsDeltaInfo) {
Iterator<byte[]> iterator = addsDeltaInfo.getAdds().iterator();
while (iterator.hasNext()) {
byte[] member = iterator.next();
- byte[] score = iterator.next();
+ double score = Coder.bytesToDouble(iterator.next());
memberAdd(member, score);
}
}
+ private synchronized void membersZAddAll(ZaddsDeltaInfo zaddsDeltaInfo) {
Review comment:
With the removal of the now-defunct `membersAddAll()` method, this method could maybe be renamed to "membersAddAll" to keep things simpler, since it doesn't make a lot of sense to specify the "Z" part in the sorted set class, where it should be implicit.
##########
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) {
Review comment:
For consistency, could either this method return a `byte[]`, or could other methods in this class that return a score also return it as `double`? The former is probably the most consistent and simplest, given we have range commands that return interleaved member and score `byte[]` if "WITHSCORES" is specified and it wouldn't be sensible to change those to return `double` rather than `byte[]` for the score now.
##########
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()) {
+ double score = scoresIterator.next();
+ byte[] member = membersIterator.next();
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
if (deltaInfo == null) {
- deltaInfo = new AddsDeltaInfo(new ArrayList<>());
+ deltaInfo = new ZaddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
Review comment:
I think this block can be refactored slightly to avoid adding to the delta if we didn't actually update the score:
```
if (scoreChanged) {
if (options.isCH()) {
changesCount++;
}
if (deltaInfo == null) {
deltaInfo = new ZaddsDeltaInfo(member, score);
} else {
deltaInfo.add(member, score);
}
}
```
##########
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");
+ }
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+ for (int i = 0; i < members.size(); i++) {
+ double score = scores.get(i);
+ if (Double.isNaN(score)) {
+ throw new NumberFormatException(ERROR_NOT_A_VALID_FLOAT);
+ }
Review comment:
With the changes to the `Coder.bytesToDouble()` method that treats "NaN" as an invalid float, it should not be possible to have `score` be NaN at this point, so this check can be removed. Sanitization of inputs should ideally be done in the Executor classes, I think, and both `ZAddExecutor` and `ZIncryByExecutor` (the only two executors that can result in a call to this constructor) have checks for invalid score values.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -648,10 +656,9 @@ public int compareScores(double score1, double score2) {
public static final int ORDERED_SET_ENTRY_OVERHEAD = memoryOverhead(OrderedSetEntry.class);
- public OrderedSetEntry(byte[] member, byte[] score) {
+ public OrderedSetEntry(byte[] member, double score) {
this.member = member;
- this.scoreBytes = stripTrailingZeroFromDouble(score);
Review comment:
With the removal of this line, the `stripTrailingZeroFromDouble()` method is not used anywhere outside of tests, so it can be removed.
##########
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);
Review comment:
Inconsistent use of `this` here. It shouldn't be necessary in this method.
##########
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<>();
Review comment:
These should be declared as the interface `List<>` rather that the concrete class, since we're never calling `DataSerializer.writeArrayList()` like in `AddsDeltaInfo`, which is the only reason the field in that class need to be an `ArrayList<>`. Doing this allows the fields to be set in the constructor directly rather than using `addAll()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -433,18 +457,19 @@ private long iteratorRangeRemove(Iterator<AbstractOrderedSetEntry> scoresIterato
return entriesRemoved;
}
- private byte[] zaddIncr(Region<RedisKey, RedisData> region, RedisKey key,
- List<byte[]> membersToAdd, ZAddOptions options) {
+ private byte[] zaddIncr(Region<RedisKey, RedisData> region, RedisKey key, byte[] member,
+ double increment, ZAddOptions options) {
+ if (Double.isNaN(increment)) {
+ throw new NumberFormatException(ERROR_NOT_A_VALID_FLOAT);
+ }
Review comment:
This check should be unnecessary, as the ZAddExecutor already checks for NaN increments (assuming the `Coder.bytesToDouble()` method keeps its NaN check that throws an Exception).
##########
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:
Internally, there is no situation in which NaN is ever a valid double value for us. If we ever get passed NaN by the user or would get NaN from a calculation, then we always return an error and never use the actual "NaN" value. As such, it makes sense to just hard-code it into our conversion method rather than adding checks for it all over the place, since there's no situation in which we would have NaN and be okay with it.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -338,24 +338,22 @@ public void getSizeInBytesIsAccurateForOrderedSetEntryValues() {
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
byte[] member = key;
- byte[] scoreBytes = String.valueOf(totalNumberOfElements - i).getBytes();
- RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, scoreBytes);
+ double score = totalNumberOfElements - i;
+ RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, score);
hash.put(key, value);
assertThat(hash.getSizeInBytes()).isEqualTo(expectedSize(hash));
}
// Update values and confirm that size changes as expected
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
- byte[] member = key;
- byte[] scoreBytes = String.valueOf(i).getBytes();
RedisSortedSet.OrderedSetEntry value = hash.get(key);
- byte[] oldScoreBytes = value.getScoreBytes();
- int scoreDelta = memoryOverhead(scoreBytes)
- - memoryOverhead(oldScoreBytes);
+ double oldScore = value.getScore();
+ int scoreDelta =
+ memoryOverhead(Coder.doubleToBytes(i)) - memoryOverhead(Coder.doubleToBytes(oldScore));
int oldSize = hash.getSizeInBytes();
- value.updateScore(scoreBytes);
+ value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isEqualTo(scoreDelta);
Review comment:
The scoreDelta here should be 0, since `double` is fixed size and updating its value does not change its size. This section can then be refactored to:
```
RedisSortedSet.OrderedSetEntry value = hash.get(key);
int oldSize = hash.getSizeInBytes();
value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isZero();
```
##########
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() {}
Review comment:
This empty constructor is never used and can be removed, or reworked to have similar behaviour to the 0-arg constructor of `RemsDeltaInfo` or the `AddsDeltaInfo(int size)` constructor for `AddsDeltaInfo`.
##########
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) {
Review comment:
Rather than this being its own method, which basically just reimplements `add()` it might be better to do what `AddsDeltaInfo` and `RemsDeltaInfo` do, and call a constructor that does not need data, then call `add()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -89,6 +101,9 @@ private int findAndValidateZAddOptions(Command command, Iterator<byte[]> command
optionsFoundCount++;
} else if (isInfinity(subCommand)) {
scoreFound = true;
+ } else if (isNaN(subCommand)) {
+ executorState.exceptionMessage = ERROR_NOT_A_VALID_FLOAT;
+ return 0;
Review comment:
With the call to `Coder.bytesToDouble()` on line 60, this check shouldn't be needed, as the modified `bytesToDouble()` method will now throw an exception with the correct message when we get to that line. We would do a couple of unnecessary allocations if a user has passed NaN as a score with this removed, but we would avoid doing a byte array comparison on every argument in the more likely case where they haven't passed NaN, so it's a net performance improvement, I think.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699681480
##########
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());
Review comment:
The initial size of `members` should be just `members.size()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -91,6 +94,9 @@ protected void applyDelta(DeltaInfo deltaInfo) {
if (deltaInfo instanceof AddsDeltaInfo) {
AddsDeltaInfo addsDeltaInfo = (AddsDeltaInfo) deltaInfo;
membersAddAll(addsDeltaInfo);
Review comment:
With the changes to this class, `AddsDeltaInfo` should no longer ever be used in this class, as it assumes an interleaved list of member names and scores, which is no longer valid. This means that this if statement and the `membersAddAll()` method can be removed.
##########
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()) {
Review comment:
This check is already made in the Executors that might call this constructor. `ZAddExecutor` and `ZIncryByExecutor` are the only two, I think, and both have logic in place to deal with member/score mismatches, so this check, and the similar one in `zadd()` can be safely removed.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -163,43 +169,56 @@ public int getDSFID() {
return REDIS_SORTED_SET_ID;
}
- protected synchronized byte[] memberAdd(byte[] memberToAdd, byte[] scoreToAdd) {
- OrderedSetEntry existingEntry = members.get(memberToAdd);
- if (existingEntry == null) {
+ protected synchronized boolean memberAdd(byte[] memberToAdd, double scoreToAdd) {
+ OrderedSetEntry entry = members.get(memberToAdd);
+ if (entry == null) {
OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
- return null;
+ return false;
} else {
- scoreSet.remove(existingEntry);
- byte[] oldScore = existingEntry.scoreBytes;
- existingEntry.updateScore(stripTrailingZeroFromDouble(scoreToAdd));
- members.put(memberToAdd, existingEntry);
- scoreSet.add(existingEntry);
- return oldScore;
+ if (entry.score == scoreToAdd) {
+ return false;
+ }
+ updateScore(memberToAdd, scoreToAdd, entry);
}
+ return true;
}
- synchronized byte[] memberRemove(byte[] member) {
- byte[] oldValue = null;
+ private void updateScore(byte[] memberToAdd, double scoreToAdd, OrderedSetEntry entry) {
+ scoreSet.remove(entry);
+ entry.updateScore(scoreToAdd);
+ members.put(memberToAdd, entry);
+ scoreSet.add(entry);
+ }
+
+ synchronized boolean memberRemove(byte[] member) {
OrderedSetEntry orderedSetEntry = members.remove(member);
if (orderedSetEntry != null) {
scoreSet.remove(orderedSetEntry);
- oldValue = orderedSetEntry.getScoreBytes();
+ return true;
}
- return oldValue;
+ return false;
}
private synchronized void membersAddAll(AddsDeltaInfo addsDeltaInfo) {
Iterator<byte[]> iterator = addsDeltaInfo.getAdds().iterator();
while (iterator.hasNext()) {
byte[] member = iterator.next();
- byte[] score = iterator.next();
+ double score = Coder.bytesToDouble(iterator.next());
memberAdd(member, score);
}
}
+ private synchronized void membersZAddAll(ZaddsDeltaInfo zaddsDeltaInfo) {
Review comment:
With the removal of the now-defunct `membersAddAll()` method, this method could maybe be renamed to "membersAddAll" to keep things simpler, since it doesn't make a lot of sense to specify the "Z" part in the sorted set class, where it should be implicit.
##########
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) {
Review comment:
For consistency, could either this method return a `byte[]`, or could other methods in this class that return a score also return it as `double`? The former is probably the most consistent and simplest, given we have range commands that return interleaved member and score `byte[]` if "WITHSCORES" is specified and it wouldn't be sensible to change those to return `double` rather than `byte[]` for the score now.
##########
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()) {
+ double score = scoresIterator.next();
+ byte[] member = membersIterator.next();
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
if (deltaInfo == null) {
- deltaInfo = new AddsDeltaInfo(new ArrayList<>());
+ deltaInfo = new ZaddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
Review comment:
I think this block can be refactored slightly to avoid adding to the delta if we didn't actually update the score:
```
if (scoreChanged) {
if (options.isCH()) {
changesCount++;
}
if (deltaInfo == null) {
deltaInfo = new ZaddsDeltaInfo(member, score);
} else {
deltaInfo.add(member, score);
}
}
```
##########
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");
+ }
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+ for (int i = 0; i < members.size(); i++) {
+ double score = scores.get(i);
+ if (Double.isNaN(score)) {
+ throw new NumberFormatException(ERROR_NOT_A_VALID_FLOAT);
+ }
Review comment:
With the changes to the `Coder.bytesToDouble()` method that treats "NaN" as an invalid float, it should not be possible to have `score` be NaN at this point, so this check can be removed. Sanitization of inputs should ideally be done in the Executor classes, I think, and both `ZAddExecutor` and `ZIncryByExecutor` (the only two executors that can result in a call to this constructor) have checks for invalid score values.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -648,10 +656,9 @@ public int compareScores(double score1, double score2) {
public static final int ORDERED_SET_ENTRY_OVERHEAD = memoryOverhead(OrderedSetEntry.class);
- public OrderedSetEntry(byte[] member, byte[] score) {
+ public OrderedSetEntry(byte[] member, double score) {
this.member = member;
- this.scoreBytes = stripTrailingZeroFromDouble(score);
Review comment:
With the removal of this line, the `stripTrailingZeroFromDouble()` method is not used anywhere outside of tests, so it can be removed.
##########
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);
Review comment:
Inconsistent use of `this` here. It shouldn't be necessary in this method.
##########
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<>();
Review comment:
These should be declared as the interface `List<>` rather that the concrete class, since we're never calling `DataSerializer.writeArrayList()` like in `AddsDeltaInfo`, which is the only reason the field in that class need to be an `ArrayList<>`. Doing this allows the fields to be set in the constructor directly rather than using `addAll()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -433,18 +457,19 @@ private long iteratorRangeRemove(Iterator<AbstractOrderedSetEntry> scoresIterato
return entriesRemoved;
}
- private byte[] zaddIncr(Region<RedisKey, RedisData> region, RedisKey key,
- List<byte[]> membersToAdd, ZAddOptions options) {
+ private byte[] zaddIncr(Region<RedisKey, RedisData> region, RedisKey key, byte[] member,
+ double increment, ZAddOptions options) {
+ if (Double.isNaN(increment)) {
+ throw new NumberFormatException(ERROR_NOT_A_VALID_FLOAT);
+ }
Review comment:
This check should be unnecessary, as the ZAddExecutor already checks for NaN increments (assuming the `Coder.bytesToDouble()` method keeps its NaN check that throws an Exception).
##########
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:
Internally, there is no situation in which NaN is ever a valid double value for us. If we ever get passed NaN by the user or would get NaN from a calculation, then we always return an error and never use the actual "NaN" value. As such, it makes sense to just hard-code it into our conversion method rather than adding checks for it all over the place, since there's no situation in which we would have NaN and be okay with it.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -338,24 +338,22 @@ public void getSizeInBytesIsAccurateForOrderedSetEntryValues() {
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
byte[] member = key;
- byte[] scoreBytes = String.valueOf(totalNumberOfElements - i).getBytes();
- RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, scoreBytes);
+ double score = totalNumberOfElements - i;
+ RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, score);
hash.put(key, value);
assertThat(hash.getSizeInBytes()).isEqualTo(expectedSize(hash));
}
// Update values and confirm that size changes as expected
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
- byte[] member = key;
- byte[] scoreBytes = String.valueOf(i).getBytes();
RedisSortedSet.OrderedSetEntry value = hash.get(key);
- byte[] oldScoreBytes = value.getScoreBytes();
- int scoreDelta = memoryOverhead(scoreBytes)
- - memoryOverhead(oldScoreBytes);
+ double oldScore = value.getScore();
+ int scoreDelta =
+ memoryOverhead(Coder.doubleToBytes(i)) - memoryOverhead(Coder.doubleToBytes(oldScore));
int oldSize = hash.getSizeInBytes();
- value.updateScore(scoreBytes);
+ value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isEqualTo(scoreDelta);
Review comment:
The scoreDelta here should be 0, since `double` is fixed size and updating its value does not change its size. This section can then be refactored to:
```
RedisSortedSet.OrderedSetEntry value = hash.get(key);
int oldSize = hash.getSizeInBytes();
value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isZero();
```
##########
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() {}
Review comment:
This empty constructor is never used and can be removed, or reworked to have similar behaviour to the 0-arg constructor of `RemsDeltaInfo` or the `AddsDeltaInfo(int size)` constructor for `AddsDeltaInfo`.
##########
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) {
Review comment:
Rather than this being its own method, which basically just reimplements `add()` it might be better to do what `AddsDeltaInfo` and `RemsDeltaInfo` do, and call a constructor that does not need data, then call `add()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -89,6 +101,9 @@ private int findAndValidateZAddOptions(Command command, Iterator<byte[]> command
optionsFoundCount++;
} else if (isInfinity(subCommand)) {
scoreFound = true;
+ } else if (isNaN(subCommand)) {
+ executorState.exceptionMessage = ERROR_NOT_A_VALID_FLOAT;
+ return 0;
Review comment:
With the call to `Coder.bytesToDouble()` on line 60, this check shouldn't be needed, as the modified `bytesToDouble()` method will now throw an exception with the correct message when we get to that line. We would do a couple of unnecessary allocations if a user has passed NaN as a score with this removed, but we would avoid doing a byte array comparison on every argument in the more likely case where they haven't passed NaN, so it's a net performance improvement, I think.
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r707477991
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/delta/ZAddsDeltaInfo.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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;
+
+public class ZAddsDeltaInfo implements DeltaInfo {
+ private final List<byte[]> deltas;
+ private final double[] scores;
+
+ public ZAddsDeltaInfo(int size) {
+ this.deltas = new ArrayList<>();
Review comment:
presize the ArrayList by passing "size" to the constructor
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +52,18 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())),
+ int size = (commandElements.size() - optionsFoundCount - 2) / 2;
+ List<byte[]> members = new ArrayList<>(size);
+ List<Double> scores = new ArrayList<>(size);
Review comment:
I still see "scores" is a List<Double>. You can change it double[] and change the "zadd" method to take a double[].
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r699433762
##########
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:
I did that to make the fields final. plus even if we just do `this.deltas = deltas`, we still have to convert deltas to an arraylist
##########
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:
we can check in the executor if the increment is NaN and exit early, but that check in RedisSortedSet is still required. it handles the case where the score is -Infinity and the increment is +Infinity, which are both valid, but adding them produces NaN
##########
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:
we can check in the executor if the increment is NaN and exit early, but that check in RedisSortedSet is still required. it handles the case where the score is -Infinity and the increment is +Infinity, which are both valid, but adding them produces NaN
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r705692218
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -818,15 +781,11 @@ protected int sizeValue(OrderedSetEntry value) {
public void toData(DataOutput out) throws IOException {
InternalDataSerializer.writePrimitiveInt(size(), out);
- final int maxIndex = getMaxIndex();
- for (int pos = 0; pos < maxIndex; ++pos) {
- OrderedSetEntry value = getValueAtIndex(pos);
- if (value != null) {
- byte[] member = value.getMember();
- byte[] score = value.getScoreBytes();
- InternalDataSerializer.writeByteArray(member, out);
- InternalDataSerializer.writeByteArray(score, out);
- }
+ for (Map.Entry<byte[], OrderedSetEntry> entry : entrySet()) {
+ byte[] member = entry.getKey();
+ double score = entry.getValue().getScore();
+ InternalDataSerializer.writeByteArray(member, out);
+ InternalDataSerializer.writeDouble(score, out);
Review comment:
sorry, thought I'd changed that
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r695936459
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -274,9 +277,9 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
+ memberAdd(member, incr);
+ byte[] byteIncr = doubleToBytes(incr);
Review comment:
use the new ZAddsDeltaInfo here to send "incr"
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -274,9 +277,9 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
+ memberAdd(member, incr);
Review comment:
This method is a bit confusing because it increments "incr" instead of incrementing "byteScore" by "incr". I think it would be easier to understand if "incr" always represented increment. I know this pr did not make this choice but if you end up working on this method more it would be nice to clean that up. It confused me at first because when I saw it adding "byteIncr" to the deltaInfo I thought it was distributing the increment but it is actually the new incremented value.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -221,25 +225,24 @@ Object zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]> membe
int initialSize = scoreSet.size();
int changesCount = 0;
while (iterator.hasNext()) {
- byte[] score = iterator.next();
+ double score = Coder.bytesToDouble(iterator.next());
byte[] member = iterator.next();
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
if (deltaInfo == null) {
deltaInfo = new AddsDeltaInfo(new ArrayList<>());
}
deltaInfo.add(member);
- deltaInfo.add(score);
+ deltaInfo.add(Coder.doubleToBytes(score));
Review comment:
don't call doubleToBytes here. You have already done the work of converting the score to a double and by shipping the bytes in the DeltaInfo that work is repeated when the delta is applied on the other side. I think you should just add a new flavor of DeltaInfo. You could name it ZAddsDeltaInfo and its add signature would be "add(byte[], double)". It does not matter that much how you store the data in the DeltaInfo but when you serialize it just write the item count (using InternalDataSerializer.writeArrayLength) and then write a byte[] (using DataSerializer.writeByteArray) and a double (using DataSerializer.writePrimitiveDouble). Also this current code does an odd thing when constructing AddsDeltaInfo. It uses a constructor that takes an ArrayList but that contructor is really meant for copying a list into the AddsDeltaInfo. Instead it should have used the AddsDeltaInfo(size) constructor and used the size of membersToAdd to as the parameter. You will want to do this for your
new ZAddsDeltaInfo
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r705648542
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
##########
@@ -64,16 +65,18 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
@Override
- byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
+ byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, double increment,
byte[] member) {
- List<byte[]> valuesToAdd = new ArrayList<>();
- valuesToAdd.add(increment);
- valuesToAdd.add(member);
+ List<byte[]> membersToAdd = new ArrayList<>();
+ membersToAdd.add(member);
- RedisSortedSet sortedSet = new RedisSortedSet(valuesToAdd);
+ List<Double> scoresToAdd = new ArrayList<>();
+ scoresToAdd.add(increment);
+
+ RedisSortedSet sortedSet = new RedisSortedSet(membersToAdd, scoresToAdd);
Review comment:
This can be simplified to
```
RedisSortedSet sortedSet = new RedisSortedSet(singletonList(member), singletonList(increment));
```
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableBytes2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -364,24 +364,22 @@ public void getSizeInBytesIsAccurateForOrderedSetEntryValues() {
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
byte[] member = key;
- byte[] scoreBytes = String.valueOf(totalNumberOfElements - i).getBytes();
- RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, scoreBytes);
+ double score = totalNumberOfElements - i;
+ RedisSortedSet.OrderedSetEntry value = new RedisSortedSet.OrderedSetEntry(member, score);
hash.put(key, value);
assertThat(hash.getSizeInBytes()).isEqualTo(sizer.sizeof(hash));
}
// Update values and confirm that size changes as expected
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
- byte[] member = key;
- byte[] scoreBytes = String.valueOf(i).getBytes();
RedisSortedSet.OrderedSetEntry value = hash.get(key);
- byte[] oldScoreBytes = value.getScoreBytes();
- int scoreDelta = memoryOverhead(scoreBytes)
- - memoryOverhead(oldScoreBytes);
+ double oldScore = value.getScore();
+ int scoreDelta =
+ memoryOverhead(Coder.doubleToBytes(i)) - memoryOverhead(Coder.doubleToBytes(oldScore));
int oldSize = hash.getSizeInBytes();
- value.updateScore(scoreBytes);
+ value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isEqualTo(scoreDelta);
Review comment:
The scoreDelta here should be 0, since `double` is fixed size and updating its value does not change its size. This section can then be refactored to:
```
RedisSortedSet.OrderedSetEntry value = hash.get(key);
int oldSize = hash.getSizeInBytes();
value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isZero();
```
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +53,17 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())),
+ List<byte[]> members = new ArrayList<>();
+ List<Double> scores = new ArrayList<>();
Review comment:
These lists can be created with appropriate sizes using something like:
```
int size = (commandElements.size() - optionsFoundCount - 2) / 2;
List<byte[]> members = new ArrayList<>(size);
List<Double> scores = new ArrayList<>(size);
```
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -105,16 +110,6 @@ public void narrowLongToInt_correctlyConvertsLongToInt() {
}
}
- @Test
- @Parameters(method = "doubleBytes")
- public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input,
Review comment:
With the removal of this test, it would be good to add an equivalent one that tests the behaviour in `Coder.doubleToBytes()` that strips trailing zeroes from double values. The currently unused `doubleBytes()` method in this test class could be fairly easily repurposed to facilitate this:
```
@Test
@Parameters(method = "doubleBytes")
public void doubleToBytes_processesTrailingZeroLikeRedis(Double inputDouble, byte[] expectedBytes) {
double d = inputDouble;
byte[] convertedBytes = doubleToBytes(d);
assertArrayEquals(convertedBytes, expectedBytes);
}
@SuppressWarnings("unused")
private Object[] doubleBytes() {
// input double, double bytes with stripped trailing zero
return new Object[] {
new Object[] {0.0, stringToBytes("0")},
new Object[] {0.01, stringToBytes("0.01")},
new Object[] {0d, stringToBytes("0")},
new Object[] {1.0, stringToBytes("1")},
new Object[] {-1.0, stringToBytes("-1")},
new Object[] {6.0221409E23, stringToBytes("6.0221409E23")},
new Object[] {6.62607E-34, stringToBytes("6.62607E-34")},
new Object[] {Double.POSITIVE_INFINITY, stringToBytes("inf")},
new Object[] {Double.NaN, stringToBytes("NaN")},
};
}
```
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -210,34 +206,34 @@ 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);
}
- AddsDeltaInfo deltaInfo = null;
- Iterator<byte[]> iterator = membersToAdd.iterator();
+
+ ZAddsDeltaInfo deltaInfo = null;
int initialSize = scoreSet.size();
int changesCount = 0;
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+
+ for (int i = 0; i < membersToAdd.size(); i++) {
+ double score = scoresToAdd.get(i);
+ byte[] member = membersToAdd.get(i);
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
-
if (deltaInfo == null) {
- deltaInfo = new AddsDeltaInfo(new ArrayList<>());
+ deltaInfo = new ZAddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
- deltaInfo.add(member);
- deltaInfo.add(score);
}
Review comment:
I had a bit more of a think about the behaviour here and have a potential solution to us sending Deltas and updating the region for no-op updates (where the score is not changed). I added an enum to the class:
```
private enum MemberAddResult {
CREATE,
UPDATE,
NO_OP
}
```
Then I modified the `memberAdd()` method:
```
protected synchronized MemberAddResult memberAdd(byte[] memberToAdd, double scoreToAdd) {
OrderedSetEntry entry = members.get(memberToAdd);
if (entry == null) {
OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
return MemberAddResult.CREATE;
} else {
if (entry.score == scoreToAdd) {
return MemberAddResult.NO_OP;
}
scoreSet.remove(entry);
entry.updateScore(scoreToAdd);
scoreSet.add(entry);
return MemberAddResult.UPDATE;
}
}
```
and finally updated the code here to make use of the new enum:
```
MemberAddResult addResult = memberAdd(member, score);
if (options.isCH() && addResult.equals(MemberAddResult.UPDATE)) {
changesCount++;
}
if (!addResult.equals(MemberAddResult.NO_OP)) {
if (deltaInfo == null) {
deltaInfo = new ZAddsDeltaInfo(member, score);
} else {
deltaInfo.add(member, score);
}
}
```
This is possibly overkill, but it does prevent us sending deltas when we don't need to, which is a small performance optimization. The approach used here (only creating/adding to `ZAddsDeltaInfo` if the result is not `NO_OP`) can also be used everywhere else `memberAdd()` is called to reduce the number of unnecessary deltas we're sending.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -147,11 +147,13 @@ public void zadd_stores_delta_that_is_stable() {
Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
when(region.put(any(), any())).thenAnswer(this::validateDeltaSerialization);
RedisSortedSet sortedSet1 = createRedisSortedSet("3.14159", "v1", "2.71828", "v2");
- List<byte[]> adds = new ArrayList<>();
- adds.add(stringToBytes("1.61803"));
- adds.add(stringToBytes("v3"));
+ List<byte[]> members = new ArrayList<>();
+ members.add(stringToBytes("v3"));
+ List<Double> scores = new ArrayList<>();
+ scores.add(1.61803);
Review comment:
This can be simplified to:
```
List<byte[]> members = singletonList(stringToBytes("v3"));
List<Double> scores = singletonList(1.61803);
```
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -411,9 +411,31 @@ public static double bytesToDouble(byte[] bytes) {
return NEGATIVE_INFINITY;
}
if (isNaN(bytes)) {
- return NaN;
+ throw new NumberFormatException();
+ }
+
+ try {
+ return stringToDouble(bytesToString(bytes));
+ } catch (NumberFormatException e) {
+ throw new NumberFormatException(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+ }
+
+ /**
+ * Redis specific manner to parse floats
+ *
+ * @param d String holding double
+ * @return Value of string
+ * @throws NumberFormatException if the double cannot be parsed
+ */
+ public static double stringToDouble(String d) {
+ if (d.equalsIgnoreCase(P_INF)) {
+ return Double.POSITIVE_INFINITY;
+ } else if (d.equalsIgnoreCase(N_INF)) {
Review comment:
This method is only called from within `Coder` (and a test class which uses it as a convenience method), where it's preceded with checks for other formats of infinity. It might be best to just make this method private or inline it into the `bytesToDouble()` method, since internally we should never need to handle the String representation of a double.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -89,6 +101,9 @@ private int findAndValidateZAddOptions(Command command, Iterator<byte[]> command
optionsFoundCount++;
} else if (isInfinity(subCommand)) {
scoreFound = true;
+ } else if (isNaN(subCommand)) {
+ executorState.exceptionMessage = ERROR_NOT_A_VALID_FLOAT;
+ return 0;
Review comment:
> With the call to `Coder.bytesToDouble()` on line 60, this check shouldn't be needed, as the modified `bytesToDouble()` method will now throw an exception with the correct message when we get to that line. We would do a couple of unnecessary allocations if a user has passed NaN as a score with this removed, but we would avoid doing a byte array comparison on every argument in the more likely case where they haven't passed NaN, so it's a net performance improvement, I think.
This comment is still relevant, I think.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r705609775
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -200,6 +202,15 @@ public void fromDelta(DataInput in) throws IOException, InvalidDeltaException {
byte[] byteArray = DataSerializer.readByteArray(in);
applyDelta(new AppendDeltaInfo(byteArray, sequence));
break;
+ case ZADDS:
+ int numMembers = DataSerializer.readPrimitiveInt(in);
+ List<byte[]> members = new ArrayList<>();
+ List<Double> scores = new ArrayList<>();
Review comment:
You should use `numMembers` to initialize the array sizes here.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -160,40 +152,44 @@ public int getDSFID() {
return REDIS_SORTED_SET_ID;
}
- protected synchronized byte[] memberAdd(byte[] memberToAdd, byte[] scoreToAdd) {
- OrderedSetEntry existingEntry = members.get(memberToAdd);
- if (existingEntry == null) {
+ protected synchronized boolean memberAdd(byte[] memberToAdd, double scoreToAdd) {
+ OrderedSetEntry entry = members.get(memberToAdd);
+ if (entry == null) {
OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd, scoreToAdd);
members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
- return null;
+ return false;
} else {
- scoreSet.remove(existingEntry);
- byte[] oldScore = existingEntry.scoreBytes;
- existingEntry.updateScore(stripTrailingZeroFromDouble(scoreToAdd));
- members.put(memberToAdd, existingEntry);
- scoreSet.add(existingEntry);
- return oldScore;
+ if (entry.score == scoreToAdd) {
+ return false;
+ }
+ updateScore(memberToAdd, scoreToAdd, entry);
}
+ return true;
+ }
+
+ private void updateScore(byte[] memberToAdd, double scoreToAdd, OrderedSetEntry entry) {
+ scoreSet.remove(entry);
+ entry.updateScore(scoreToAdd);
+ members.put(memberToAdd, entry);
Review comment:
I think this line is not necessary since this is a reference to the value already in the `members` map. Could you also rather inline the remaining 3 lines since this method isn't used anywhere else.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -818,15 +781,11 @@ protected int sizeValue(OrderedSetEntry value) {
public void toData(DataOutput out) throws IOException {
InternalDataSerializer.writePrimitiveInt(size(), out);
- final int maxIndex = getMaxIndex();
- for (int pos = 0; pos < maxIndex; ++pos) {
- OrderedSetEntry value = getValueAtIndex(pos);
- if (value != null) {
- byte[] member = value.getMember();
- byte[] score = value.getScoreBytes();
- InternalDataSerializer.writeByteArray(member, out);
- InternalDataSerializer.writeByteArray(score, out);
- }
+ for (Map.Entry<byte[], OrderedSetEntry> entry : entrySet()) {
+ byte[] member = entry.getKey();
+ double score = entry.getValue().getScore();
+ InternalDataSerializer.writeByteArray(member, out);
+ InternalDataSerializer.writeDouble(score, out);
Review comment:
Prefer `DataSerializer` instead of `InternalDataSerializer`
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -411,9 +411,31 @@ public static double bytesToDouble(byte[] bytes) {
return NEGATIVE_INFINITY;
}
if (isNaN(bytes)) {
- return NaN;
+ throw new NumberFormatException();
+ }
+
+ try {
+ return stringToDouble(bytesToString(bytes));
+ } catch (NumberFormatException e) {
+ throw new NumberFormatException(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+ }
+
+ /**
+ * Redis specific manner to parse floats
+ *
+ * @param d String holding double
+ * @return Value of string
+ * @throws NumberFormatException if the double cannot be parsed
+ */
+ public static double stringToDouble(String d) {
+ if (d.equalsIgnoreCase(P_INF)) {
+ return Double.POSITIVE_INFINITY;
+ } else if (d.equalsIgnoreCase(N_INF)) {
Review comment:
Should this also handle the longer form strings `[+-]Infinfity`?
--
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
[GitHub] [geode] nonbinaryprogrammer merged pull request #6783: GEODE-9446: Remove unecessary byte arrays from RedisSortedSet
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #6783:
URL: https://github.com/apache/geode/pull/6783
--
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