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/03/16 15:02:26 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6146: [DRAFT] Refactor all redis key references to use RedisKey
jdeppe-pivotal opened a new pull request #6146:
URL: https://github.com/apache/geode/pull/6146
- RedisKey extends ByteArrayWrapper and adds a routingId integer value.
This is used by the RedisPartitionResolver to map entries to buckets.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597154751
##########
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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
+ }
+
+ @Override
+ public int getDSFID() {
+ return DataSerializableFixedID.REDIS_KEY;
+ }
+
+ @Override
+ public void toData(DataOutput out, SerializationContext context) throws IOException {
+ out.writeInt(crc16);
+ super.toData(out, context);
+ }
+
+ @Override
+ public void fromData(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ crc16 = in.readInt();
+ super.fromData(in, context);
+ }
+
+ @Override
+ public KnownVersion[] getSerializationVersions() {
+ return null;
+ }
+
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets
+ */
+ @Override
+ public int hashCode() {
+ return crc16;
Review comment:
Good point. Probably worth adding that `crc16()` method back (though not with an `Integer` return type) for the `ParitionResolver` to use and fall back to the default `hashCode()` here.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597983886
##########
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:
I'm not quite sure about your concern. There are 10 sample inputs in `RedisKeyJUnitTest.testRoutingId_withHashtags`. Could you express a relevant test case in the form of an actual input example?
--
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
[GitHub] [geode] lgtm-com[bot] commented on pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6146:
URL: https://github.com/apache/geode/pull/6146#issuecomment-802190748
This pull request **introduces 1 alert** when merging 6f0f4d480c85d7399c480489dbe48b8b4e8d72a8 into 3b422bbe9631b7531633671a3938ed9600cfbc6e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-0bacb5b15e806407d13508cdc9ecfb20ea279fd8)
**new alerts:**
* 1 for Inconsistent equals and hashCode
--
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
[GitHub] [geode] upthewaterspout commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597156174
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -15,16 +15,20 @@
package org.apache.geode.redis.internal.executor.cluster;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
import org.apache.geode.cache.EntryOperation;
import org.apache.geode.cache.PartitionResolver;
-import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
-public class RedisPartitionResolver implements PartitionResolver<ByteArrayWrapper, RedisData> {
+public class RedisPartitionResolver implements PartitionResolver<RedisKey, RedisData> {
@Override
- public Object getRoutingObject(EntryOperation<ByteArrayWrapper, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ return opDetails.getKey().hashCode() & (REDIS_SLOTS - 1) / REDIS_SLOTS_PER_BUCKET;
Review comment:
Isn't the hashcode a 16 bit CRC? So doing `& (REDIS_SLOTS - 1)` is a noop? You are just taking the lower 16 bits of a 16 bit value? Maybe just doing this for safety's sake?
--
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
[GitHub] [geode] Bill commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r598020223
##########
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:
Oh I see @jdeppe-pivotal. I looked at that first assertion and stopped reading.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597982337
##########
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:
Ditto above.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r598741253
##########
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:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597153103
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -15,16 +15,20 @@
package org.apache.geode.redis.internal.executor.cluster;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
import org.apache.geode.cache.EntryOperation;
import org.apache.geode.cache.PartitionResolver;
-import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
-public class RedisPartitionResolver implements PartitionResolver<ByteArrayWrapper, RedisData> {
+public class RedisPartitionResolver implements PartitionResolver<RedisKey, RedisData> {
@Override
- public Object getRoutingObject(EntryOperation<ByteArrayWrapper, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
Review comment:
No, it isn't the reason. The reason for powers of 2 is to maintain an even distribution of slots to buckets. If buckets is not a factor of slots then some buckets will hold more slots than others, leading to an imbalance of data.
--
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
[GitHub] [geode] upthewaterspout commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597147632
##########
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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
+ }
+
+ @Override
+ public int getDSFID() {
+ return DataSerializableFixedID.REDIS_KEY;
+ }
+
+ @Override
+ public void toData(DataOutput out, SerializationContext context) throws IOException {
+ out.writeInt(crc16);
+ super.toData(out, context);
+ }
+
+ @Override
+ public void fromData(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ crc16 = in.readInt();
+ super.fromData(in, context);
+ }
+
+ @Override
+ public KnownVersion[] getSerializationVersions() {
+ return null;
+ }
+
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets
+ */
+ @Override
+ public int hashCode() {
+ return crc16;
Review comment:
We don't want to use this as the hashCode! The crc16 is based on the hashtag, which is used to group keys into a single slot. A lot of keys are going to have the same hashtag, that's the point. So a lot of keys are going to have the same crc16 in this case.
The hashCode is used when adding the entry to the ConcurrentHashMap of the bucket. So if all of the keys have the same hashCode, our hashtable be a linked list.
Even the CRC16 of the whole key could be a bad idea, because we are creating more collisions by only having a 16 bit hashcode instead of a 32 bit one.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -15,16 +15,20 @@
package org.apache.geode.redis.internal.executor.cluster;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
import org.apache.geode.cache.EntryOperation;
import org.apache.geode.cache.PartitionResolver;
-import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
-public class RedisPartitionResolver implements PartitionResolver<ByteArrayWrapper, RedisData> {
+public class RedisPartitionResolver implements PartitionResolver<RedisKey, RedisData> {
@Override
- public Object getRoutingObject(EntryOperation<ByteArrayWrapper, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
Review comment:
You made my brain explode :) Is this why we are restricting the number of buckets to a power of 2? That seems not worth doing just to use & instead of / here.
--
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
[GitHub] [geode] Bill commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r598018423
##########
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:
Looks like your test method draws directly from the spec in https://redis.io/topics/cluster-spec which is great!
--
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
[GitHub] [geode] Bill commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by 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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597709008
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -56,33 +52,31 @@ public RedisKey(byte[] value) {
endHashtag = value.length;
}
- // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
- routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
- / REDIS_SLOTS_PER_BUCKET;
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
}
/**
* Used by the {@link RedisPartitionResolver} to map slots to buckets
*/
- public Object getRoutingId() {
- return routingId;
+ public Integer getCrc16() {
+ return crc16;
}
@Override
public int getDSFID() {
- return DataSerializableFixedID.REDIS_BYTE_ARRAY_WRAPPER_KEY;
+ return DataSerializableFixedID.REDIS_KEY;
}
@Override
public void toData(DataOutput out, SerializationContext context) throws IOException {
- DataSerializer.writeInteger(routingId, out);
+ out.writeInt(crc16);
Review comment:
Yup - 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r598018193
##########
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:
oh yeah ok, I just ran this with coverage in IJ and confirmed all branches except `getSerializationVersions()` are covered. I just didn't see `testRoutingId_withHashtags()` for some reason!
--
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
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597166580
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -15,16 +15,20 @@
package org.apache.geode.redis.internal.executor.cluster;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
import org.apache.geode.cache.EntryOperation;
import org.apache.geode.cache.PartitionResolver;
-import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
-public class RedisPartitionResolver implements PartitionResolver<ByteArrayWrapper, RedisData> {
+public class RedisPartitionResolver implements PartitionResolver<RedisKey, RedisData> {
@Override
- public Object getRoutingObject(EntryOperation<ByteArrayWrapper, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ return opDetails.getKey().hashCode() & (REDIS_SLOTS - 1) / REDIS_SLOTS_PER_BUCKET;
Review comment:
Slots in 14 bits, this masks off the top 2 bits.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597265093
##########
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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
+ }
+
+ @Override
+ public int getDSFID() {
+ return DataSerializableFixedID.REDIS_KEY;
+ }
+
+ @Override
+ public void toData(DataOutput out, SerializationContext context) throws IOException {
+ out.writeInt(crc16);
+ super.toData(out, context);
+ }
+
+ @Override
+ public void fromData(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ crc16 = in.readInt();
+ super.fromData(in, context);
+ }
+
+ @Override
+ public KnownVersion[] getSerializationVersions() {
+ return null;
+ }
+
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets
+ */
+ @Override
+ public int hashCode() {
+ return crc16;
Review comment:
Switched back
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597060104
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+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 Integer routingId;
+
+ 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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6146:
URL: https://github.com/apache/geode/pull/6146
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597983975
##########
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:
I think this is covered in the test inputs in `testRoutingId_withHashtags`
--
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
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r596389114
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+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 Integer routingId;
+
+ 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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
Review comment:
Why not keep and re-use the calculated CRC16 for `int hashCode()`?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+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 Integer routingId;
+
+ 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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
Review comment:
Calculating a `routingId` and serializing it not only does this key need to know something about the number of buckets and slots / buckets but if the buckets changed all the key objects have to change. If instead we keep just the CRC16 hash and let `PartitionResolver` calculate the slot/buck from the has we can decouple this.
----------------------------------------------------------------
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
[GitHub] [geode] pivotal-jbarrett commented on pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #6146:
URL: https://github.com/apache/geode/pull/6146#issuecomment-802361566
> Looks good. With regard to caching the crc16 in the region entry - I agree it probably doesn't need to be serialized. I'm also not sure whether it would be more efficient to cache the hashCode or the CRC16 - I wish we didn't call the PartitionResolver more than once!
At some point there is a tipping point were the serialization of 2 bytes is cheaper than the CPU time to recalculate the CRC remotely. Keeping in mind that for Geode that CPU is also trying to process primary updates too. Or would it even need to be calculated remotely if since it is only used for bucket placement, and that is already known when replicating.
Similar question can likely be said for the hash code's 4 bytes. Since to be placed in to correct map slot remotely the hash code must be known so is recalculation on the remote side more costly in CPU than the 4 bytes on the wire?
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597982282
##########
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:
The CRC16 value is really an unsigned short. Since shorts in Java are signed, we're holding it in an int. However, when serializing on the the wire, it is sent as a short to save some bits. When deserializing we `& 0xffff` the short to get it back to an `int` and effectively an unsigned short value. I wanted to make sure that the `& 0xffff` bit worked and the semantics of that were codified in a test.
--
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
[GitHub] [geode] jdeppe-pivotal commented on pull request #6146: [DRAFT] Refactor all redis key references to use RedisKey
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6146:
URL: https://github.com/apache/geode/pull/6146#issuecomment-800339664
I tried to break this up into one commit which has the 'actual' change and the other is switching usage of `ByteArrayWrapper` to `RedisKey` where appropriate.
----------------------------------------------------------------
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
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597735980
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -24,7 +27,8 @@
@Override
public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ return opDetails.getKey().getCrc16() & (REDIS_SLOTS - 1) / REDIS_SLOTS_PER_BUCKET;
Review comment:
> It was switched away from being a ^2.
Just for clarification, it is still a power of 2 and the recommendation in the comment is a power of 2 by looking at the source. I don't recall any conversation where we agreed to not have it be a power of 2.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597709731
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -24,7 +27,8 @@
@Override
public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ return opDetails.getKey().getCrc16() & (REDIS_SLOTS - 1) / REDIS_SLOTS_PER_BUCKET;
Review comment:
There already was a conversation about the bucket number. It was switched away from being a ^2.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597983886
##########
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:
I'm not quite sure about your concern. There are 10 sample inputs in `testRoutingId_withHashtags`. Could you express a relevant test case in the form of an actual input example?
##########
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:
Ditto
##########
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:
Ditto
--
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
[GitHub] [geode] lgtm-com[bot] commented on pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6146:
URL: https://github.com/apache/geode/pull/6146#issuecomment-801970505
This pull request **introduces 1 alert** when merging 20de8cfae4060500dce8cbddd8a3d753a6e0162b into 14e1cfca58f9efb2b9bde0e51e5d1671ff6e9745 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-8fae08419f37d84cad1a70ecd9670a226b6b1b13)
**new alerts:**
* 1 for Inconsistent equals and hashCode
----------------------------------------------------------------
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597710067
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+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 Integer routingId;
+
+ 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;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597150751
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -15,16 +15,20 @@
package org.apache.geode.redis.internal.executor.cluster;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS;
+import static org.apache.geode.redis.internal.RegionProvider.REDIS_SLOTS_PER_BUCKET;
+
import org.apache.geode.cache.EntryOperation;
import org.apache.geode.cache.PartitionResolver;
-import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisKey;
-public class RedisPartitionResolver implements PartitionResolver<ByteArrayWrapper, RedisData> {
+public class RedisPartitionResolver implements PartitionResolver<RedisKey, RedisData> {
@Override
- public Object getRoutingObject(EntryOperation<ByteArrayWrapper, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
Review comment:
You made my brain explode :) Is this why we are restricting the number of buckets to a power of 2? That seems not worth doing just to use & instead of / here.
--
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
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597067742
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -56,33 +52,31 @@ public RedisKey(byte[] value) {
endHashtag = value.length;
}
- // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
- routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
- / REDIS_SLOTS_PER_BUCKET;
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
}
/**
* Used by the {@link RedisPartitionResolver} to map slots to buckets
*/
- public Object getRoutingId() {
- return routingId;
+ public Integer getCrc16() {
+ return crc16;
}
@Override
public int getDSFID() {
- return DataSerializableFixedID.REDIS_BYTE_ARRAY_WRAPPER_KEY;
+ return DataSerializableFixedID.REDIS_KEY;
}
@Override
public void toData(DataOutput out, SerializationContext context) throws IOException {
- DataSerializer.writeInteger(routingId, out);
+ out.writeInt(crc16);
Review comment:
Technically it is an unsigned short, could we not write it as short field and save some bits?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -59,6 +59,11 @@ public static int calculate(byte[] data, int start, int end) {
return crc;
}
+ public static int calculate(String data) {
+ byte[] bytes = data.getBytes();
Review comment:
Is this method frequency invoked? If so we should produce a version that can CRC-16 the character sequence assuming it is ASCII and avoid the transient allocation of a `byte[]` and the conversion.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/RedisPartitionResolver.java
##########
@@ -24,7 +27,8 @@
@Override
public Object getRoutingObject(EntryOperation<RedisKey, RedisData> opDetails) {
- return opDetails.getKey().getRoutingId();
+ // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
+ return opDetails.getKey().getCrc16() & (REDIS_SLOTS - 1) / REDIS_SLOTS_PER_BUCKET;
Review comment:
Can we confirm that the compiler computes `REDIS_SLOTS - 1` otherwise we should produce the mask explicitly.
Also faster is that if the buckets are powers of 2 then bit shifting rather than dividing.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java
##########
@@ -56,33 +52,31 @@ public RedisKey(byte[] value) {
endHashtag = value.length;
}
- // & (REDIS_SLOTS - 1) is equivalent to % REDIS_SLOTS but supposedly faster
- routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) & (REDIS_SLOTS - 1))
- / REDIS_SLOTS_PER_BUCKET;
+ crc16 = CRC16.calculate(value, startHashtag + 1, endHashtag);
}
/**
* Used by the {@link RedisPartitionResolver} to map slots to buckets
*/
- public Object getRoutingId() {
- return routingId;
+ public Integer getCrc16() {
+ return crc16;
}
@Override
public int getDSFID() {
- return DataSerializableFixedID.REDIS_BYTE_ARRAY_WRAPPER_KEY;
+ return DataSerializableFixedID.REDIS_KEY;
}
@Override
public void toData(DataOutput out, SerializationContext context) throws IOException {
- DataSerializer.writeInteger(routingId, out);
+ out.writeInt(crc16);
super.toData(out, context);
}
@Override
public void fromData(DataInput in, DeserializationContext context)
throws IOException, ClassNotFoundException {
- routingId = DataSerializer.readInteger(in);
+ crc16 = in.readInt();
Review comment:
Wondering out loud now if it is even worth serializing the CRC when we could just reconstitute it when deserializing.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6146: GEODE-9044: Introduce RedisKey as key object for RedisData entries
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6146:
URL: https://github.com/apache/geode/pull/6146#discussion_r597378405
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -59,6 +59,11 @@ public static int calculate(byte[] data, int start, int end) {
return crc;
}
+ public static int calculate(String data) {
+ byte[] bytes = data.getBytes();
Review comment:
This is here just for test convenience.
--
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