[GitHub] [geode] Bill commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries

[GitBox <gi...@apache.org>]

Bill commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597974606



##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisKeyJUnitTest.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.assertj.core.api.Assertions.assertThat;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.internal.serialization.ByteArrayDataInput;
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+import org.apache.geode.redis.internal.executor.cluster.CRC16;
+
+public class RedisKeyJUnitTest {
+
+  @BeforeClass
+  public static void classSetup() {
+    InternalDataSerializer.getDSFIDSerializer()
+        .registerDSFID(DataSerializableFixedID.REDIS_KEY, RedisKey.class);
+  }
+
+  @Test
+  public void testRoutingId_withHashtags() {
+    RedisKey key = new RedisKey("name{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("{user1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+
+    key = new RedisKey("}user1000{".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("}user1000{"));
+
+    key = new RedisKey("user{}1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user{}1000"));
+
+    key = new RedisKey("user}{1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user}{1000"));
+
+    key = new RedisKey("{user1000}}bar".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("foo{user1000}{bar}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("foo{}{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("foo{}{user1000}"));
+
+    key = new RedisKey("{}{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{}{user1000}"));
+
+    key = new RedisKey("foo{{user1000}}bar".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+  }
+
+  @Test
+  public void testSerialization_withPositiveSignedShortCRC16() throws Exception {
+    RedisKey keyOut = new RedisKey("012345".getBytes());
+    assertThat((short) keyOut.getCrc16()).isPositive();

Review comment:
       It's not clear why this CRC value should be positive. Since we are trying to verify that the CRC16-XMODEM was correctly computed, this should test:
   
   ```
   assertThat((short) keyOut.getCrc16()).isEqualTo((short)31712);
   ```

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+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 extends ByteArrayWrapper implements DataSerializableFixedID {
+
+  private int crc16;
+
+  public RedisKey() {}
+
+  public RedisKey(byte[] value) {
+    super(value);
+
+    int startHashtag = Integer.MAX_VALUE;
+    int endHashtag = 0;
+
+    for (int i = 0; i < value.length; i++) {
+      if (value[i] == '{' && startHashtag == Integer.MAX_VALUE) {

Review comment:
       Missing test case for this branch.

##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisKeyJUnitTest.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.assertj.core.api.Assertions.assertThat;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.HeapDataOutputStream;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.internal.serialization.ByteArrayDataInput;
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+import org.apache.geode.redis.internal.executor.cluster.CRC16;
+
+public class RedisKeyJUnitTest {
+
+  @BeforeClass
+  public static void classSetup() {
+    InternalDataSerializer.getDSFIDSerializer()
+        .registerDSFID(DataSerializableFixedID.REDIS_KEY, RedisKey.class);
+  }
+
+  @Test
+  public void testRoutingId_withHashtags() {
+    RedisKey key = new RedisKey("name{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("{user1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+
+    key = new RedisKey("}user1000{".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("}user1000{"));
+
+    key = new RedisKey("user{}1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user{}1000"));
+
+    key = new RedisKey("user}{1000".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user}{1000"));
+
+    key = new RedisKey("{user1000}}bar".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("foo{user1000}{bar}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("user1000"));
+
+    key = new RedisKey("foo{}{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("foo{}{user1000}"));
+
+    key = new RedisKey("{}{user1000}".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{}{user1000}"));
+
+    key = new RedisKey("foo{{user1000}}bar".getBytes());
+    assertThat(key.getCrc16()).isEqualTo(CRC16.calculate("{user1000"));
+  }
+
+  @Test
+  public void testSerialization_withPositiveSignedShortCRC16() throws Exception {
+    RedisKey keyOut = new RedisKey("012345".getBytes());
+    assertThat((short) keyOut.getCrc16()).isPositive();
+
+    HeapDataOutputStream out = new HeapDataOutputStream(100);
+    DataSerializer.writeObject(keyOut, out);
+    ByteArrayDataInput in = new ByteArrayDataInput(out.toByteArray());
+
+    RedisKey keyIn = DataSerializer.readObject(in);
+    assertThat(keyIn).isEqualTo(keyOut);
+  }
+
+  @Test
+  public void testSerialization_withNegativeSignedShortCRC16() throws Exception {
+    RedisKey keyOut = new RedisKey("k2".getBytes());
+    assertThat((short) keyOut.getCrc16()).isNegative();

Review comment:
       It's not clear why this CRC value should be negative. Since we are trying to verify that the CRC16-XMODEM was correctly computed, this should test:
   
   ```
       assertThat((short) keyOut.getCrc16()).isEqualTo((short)49601);
   ```

##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedID.java
##########
@@ -461,7 +461,9 @@
   short UPDATE_ENTRY_VERSION_MESSAGE = 158;
   short PR_UPDATE_ENTRY_VERSION_MESSAGE = 159;
 
-  // 160 through 164 unused
+  short REDIS_KEY = 160;
+
+  // 161 through 164 unused

Review comment:
       ok ✓

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+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 extends ByteArrayWrapper implements DataSerializableFixedID {
+
+  private int crc16;
+
+  public RedisKey() {}
+
+  public RedisKey(byte[] value) {
+    super(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;
+      }
+    }

Review comment:
       Missing test cases for mis-matched hash tag delimiters e.g.:
   
   `{xyz`
   `pdq}`
   `}{`
   

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+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 extends ByteArrayWrapper implements DataSerializableFixedID {
+
+  private int crc16;
+
+  public RedisKey() {}
+
+  public RedisKey(byte[] value) {
+    super(value);
+
+    int startHashtag = Integer.MAX_VALUE;
+    int endHashtag = 0;
+
+    for (int i = 0; i < value.length; i++) {

Review comment:
       missing test case for empty array

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.internal.serialization.DataSerializableFixedID;
+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 extends ByteArrayWrapper implements DataSerializableFixedID {
+
+  private int crc16;
+
+  public RedisKey() {}
+
+  public RedisKey(byte[] value) {
+    super(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] == '}') {

Review comment:
       Missing test case for this branch.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org