dschneider-pivotal commented on a change in pull request #6847:
URL: https://github.com/apache/geode/pull/6847#discussion_r704611221
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -29,41 +28,23 @@
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
-import org.apache.geode.redis.internal.executor.cluster.CRC16;
import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RedisKey implements DataSerializableFixedID {
- private int crc16;
+ private int slot;
private byte[] value;
public RedisKey() {}
public RedisKey(byte[] value) {
this.value = value;
- int startHashtag = Integer.MAX_VALUE;
- int endHashtag = 0;
-
- for (int i = 0; i < value.length; i++) {
- if (value[i] == '{' && startHashtag == Integer.MAX_VALUE) {
- startHashtag = i;
- } else if (value[i] == '}') {
- endHashtag = i;
- break;
- }
- }
-
- if (endHashtag - startHashtag <= 1) {
- startHashtag = -1;
- endHashtag = value.length;
- }
-
- crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
+ slot = KeyHashUtil.slotForKey(value);
}
public int getBucketId() {
// & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
Review comment:
remove this comment since it no longer does & (REDIS_SLOTS - 1)
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -92,15 +73,15 @@ public int getDSFID() {
@Override
public void toData(DataOutput out, SerializationContext context) throws
IOException {
- out.writeShort(crc16);
+ out.writeShort(slot);
DataSerializer.writeByteArray(value, out);
}
@Override
public void fromData(DataInput in, DeserializationContext context)
throws IOException {
// Need to convert a signed short to unsigned
- crc16 = in.readShort() & 0xffff;
+ slot = in.readShort() & 0xffff;
Review comment:
since slot will never be negative I think you can drop the "& 0xffff".
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/KeyHashUtil.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.data;
+
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+
+import org.apache.geode.redis.internal.executor.cluster.CRC16;
+
+/**
+ * Utility for parsing the hashtags in a redis key and computing the redis
slot of the key.
+ */
+public class KeyHashUtil {
+
+ /**
+ * Compute the slot of a given redis key
+ *
+ * @return the slot, a value between 0 and 16K.
+ */
+ public static int slotForKey(byte[] value) {
+ int startHashtag = Integer.MAX_VALUE;
+ int endHashtag = 0;
+
+ for (int i = 0; i < value.length; i++) {
Review comment:
If you have a key that looks like this "aaa}bbb{tag}ccc" this parsing
will ignore the "{tag}" and say the whole key is the hash tag. Is that how
native redis also does it?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/KeyHashUtil.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.data;
+
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+
+import org.apache.geode.redis.internal.executor.cluster.CRC16;
+
+/**
+ * Utility for parsing the hashtags in a redis key and computing the redis
slot of the key.
+ */
+public class KeyHashUtil {
+
+ /**
+ * Compute the slot of a given redis key
+ *
+ * @return the slot, a value between 0 and 16K.
+ */
+ public static int slotForKey(byte[] value) {
Review comment:
since your comments refer to a "redis key" should the parameter be named
"key" instead of "value"?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -29,41 +28,23 @@
import org.apache.geode.internal.serialization.DeserializationContext;
import org.apache.geode.internal.serialization.KnownVersion;
import org.apache.geode.internal.serialization.SerializationContext;
-import org.apache.geode.redis.internal.executor.cluster.CRC16;
import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RedisKey implements DataSerializableFixedID {
- private int crc16;
+ private int slot;
Review comment:
did you consider changing this field to "short" since its value will
always be in the range 0..REDIS_SLOTS-1?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/parameters/ClusterParameterRequirements.java
##########
@@ -27,7 +27,7 @@ public void checkParameters(Command command,
ExecutionHandlerContext context) {
if (numberOfArguments < 2) {
throw new
RedisParametersMismatchException(command.wrongNumberOfArgumentsErrorMessage());
- } else if (numberOfArguments > 2) {
+ } else if (numberOfArguments > 3) {
Review comment:
It is not clear to me why this change was made. If it was wrong before
should a test have been broken? Can we add a test to the integration test that
passes CLUSTER the wrong number of parameters?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -206,7 +206,7 @@ public RedisData getRedisData(RedisKey key, RedisData
notFoundValue) {
private RedisDataMovedException createRedisDataMovedException(RedisKey key) {
RedisMemberInfo memberInfo = getRedisMemberInfo(key);
- Integer slot = key.getCrc16() & (REDIS_SLOTS - 1);
+ Integer slot = key.getSlot();
Review comment:
change "Integer" to "int" here and on RedisDataMovedException
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisKeyJUnitTest.java
##########
@@ -39,43 +40,45 @@ public static void classSetup() {
@Test
public void testRoutingId_withHashtags() {
RedisKey key = new RedisKey(stringToBytes("name{user1000}"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("user1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("{user1000"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("{user1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("}user1000{"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("}user1000{"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("}user1000{") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("user{}1000"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user{}1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("user{}1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("user}{1000"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user}{1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("user}{1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("{user1000}}bar"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("user1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("foo{user1000}{bar}"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("user1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("foo{}{user1000}"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("foo{}{user1000}"));
+ assertThat(key.getSlot())
+ .isEqualTo(CRC16.calculate("foo{}{user1000}") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("{}{user1000}"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{}{user1000}"));
+ assertThat(key.getSlot())
+ .isEqualTo(CRC16.calculate("{}{user1000}") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(stringToBytes("foo{{user1000}}bar"));
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("{user1000") %
RegionProvider.REDIS_SLOTS);
key = new RedisKey(new byte[] {});
- assertThat(key.getCrc16()).isEqualTo(CRC16.calculate(""));
+ assertThat(key.getSlot()).isEqualTo(CRC16.calculate("") %
RegionProvider.REDIS_SLOTS);
}
@Test
public void testSerialization_withPositiveSignedShortCRC16() throws
Exception {
RedisKey keyOut = new RedisKey(stringToBytes("012345"));
- assertThat((short) keyOut.getCrc16()).isPositive();
+ assertThat((short) keyOut.getSlot()).isPositive();
Review comment:
should this just be "testSerialization" and not worry about the slot
being positive as a short? If we wanted to test that slot is always >= 0 it
seems like it should be in its own test method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
