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/11 14:49:34 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6117: GEODE-9023: Add sharding support to redis region
jdeppe-pivotal opened a new pull request #6117:
URL: https://github.com/apache/geode/pull/6117
This ability allows for the future mapping of cluster slots to geode
buckets which is preemptive to enabling redis clustering support.
- Bucket count defaults to 128 and must be a a power of 2 and a factor
of 16384 (the number of redis slots).
- Supports using hashtags in the key.
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] jdeppe-pivotal commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593471220
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +65,23 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBuckets(int buckets) {
Review comment:
Thanks - fixed.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787978
##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.internal.cache.LocalDataSet;
+import org.apache.geode.redis.internal.RegionProvider;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.SerializableCallableIF;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class RedisPartitionResolverDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule cluster = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
+
+ private static int redisServerPort1;
Review comment:
Just removed and inlined the call.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787683
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
Made constant.
----------------------------------------------------------------
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] DonalEvans commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592548532
##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.internal.cache.LocalDataSet;
+import org.apache.geode.redis.internal.RegionProvider;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.SerializableCallableIF;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class RedisPartitionResolverDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule cluster = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
+
+ private static int redisServerPort1;
Review comment:
This can be converted to a local variable in `classSetup()` since it's not used outside of that method.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
return arrayCmp(value, other.value);
}
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+ * in the same way that redis does.
+ *
+ * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+ */
+ public synchronized Object getRoutingId() {
+ if (routingId == null && value != null) {
+ 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;
+ }
+ if (value[i] == '}') {
Review comment:
This could be an `else if` since it's not possible for `value[i]` to be equal to both '{' and '}'.
##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.internal.cache.LocalDataSet;
+import org.apache.geode.redis.internal.RegionProvider;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.SerializableCallableIF;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class RedisPartitionResolverDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule cluster = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
+
+ private static int redisServerPort1;
+ private static int redisServerPort2;
+ private static int redisServerPort3;
Review comment:
These two fields are never used, and can be safely removed, along with the calls to `cluster.getRedisPort()` initializing them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6117:
URL: https://github.com/apache/geode/pull/6117#issuecomment-796793215
I have another PR, based on this one, which introduces the `RedisKey` object. That PR will refactor some of the routingId calculation.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787415
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -8149,8 +8149,8 @@ public void dumpSelfEntryFromAllPartitionedRegions() {
* A test method to get the list of all the bucket ids for the partitioned region in the data
* Store.
*/
- public List getLocalBucketsListTestOnly() {
- List localBucketList = null;
+ public List<Integer> getLocalBucketsListTestOnly() {
Review comment:
Unfortunately no - it's used in quite a few other places.
----------------------------------------------------------------
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] sabbey37 commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592572832
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
Review comment:
Do we need to have some check to ensure that if `redis.region.buckets` is set via a system property that it is a power of 2 and a factor of 16384?
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593464155
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +67,24 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBucketCount(int buckets) {
+ if (buckets <= 0 || ((buckets & (buckets - 1)) != 0)) {
+ throw new ManagementException(
+ String.format(
+ "Could not start Redis Server - System property '%s' must be a power of 2. Configured value is invalid: %d",
Review comment:
If the slots count, fixed at 16k, is not evenly divisible by the bucket count then you end up with less slots in some buckets. If the data is evenly distributed across the buckets, as described in the Redis docs by use of CRC16, then data would be imbalanced too.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6117:
URL: https://github.com/apache/geode/pull/6117
----------------------------------------------------------------
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] nabarunnag commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592517006
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.executor.cluster;
+
+public class CRC16 {
+
+ // CCITT/SDLC/HDLC x^16 + x^12 + x^5 + 1 (CRC-16-CCITT)
+ private static final int CCITT_POLY = 0x8408;
+ private static final short[] crcTable = new short[256];
+
+ // Create the table up front
+ static {
Review comment:
This table will never change, will it be ok just to have a hardcoded table ?
----------------------------------------------------------------
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] sabbey37 commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592572832
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
Review comment:
Do we need to have some check to ensure that if `redis.region.buckets` is set via a system property that it is a power of 2 and a factor of 16384? (maybe that's coming in the next PR?)
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
I'm probably missing something obvious, but I was wondering why we try to get `redis.slots` via System.getProperty... will we allow it to be configured at some point? The Redis cluster docs seem to say there are always `16384` slots.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787765
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
return arrayCmp(value, other.value);
}
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+ * in the same way that redis does.
+ *
+ * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+ */
+ public synchronized Object getRoutingId() {
+ if (routingId == null && value != null) {
+ 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;
+ }
+ if (value[i] == '}') {
Review comment:
Thanks!
##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.internal.cache.LocalDataSet;
+import org.apache.geode.redis.internal.RegionProvider;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.SerializableCallableIF;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class RedisPartitionResolverDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule cluster = new RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static Jedis jedis1;
+
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
+
+ private static int redisServerPort1;
+ private static int redisServerPort2;
+ private static int redisServerPort3;
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 commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593466780
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -39,6 +45,8 @@
*/
protected byte[] value;
+ private transient Object routingId;
Review comment:
Seems like it is called very often - certainly more than once / key
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592440110
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -8149,8 +8149,8 @@ public void dumpSelfEntryFromAllPartitionedRegions() {
* A test method to get the list of all the bucket ids for the partitioned region in the data
* Store.
*/
- public List getLocalBucketsListTestOnly() {
- List localBucketList = null;
+ public List<Integer> getLocalBucketsListTestOnly() {
Review comment:
Can this be package protected at least?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
Why is this value configurable? It is a fixed value in Redis.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
Review comment:
If this is configurable we should validate that the user provides a power of 2, probably between 2 and 16384. We could even do that by changing this to the exponent of the power of 2 rather than the value.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
return arrayCmp(value, other.value);
}
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+ * in the same way that redis does.
+ *
+ * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+ */
+ public synchronized Object getRoutingId() {
+ if (routingId == null && value != null) {
+ 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;
+ }
+ if (value[i] == '}') {
+ endHashtag = i;
+ break;
+ }
+ }
+
+ if (endHashtag - startHashtag <= 1) {
+ startHashtag = -1;
+ endHashtag = value.length;
+ }
+
+ routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) % REDIS_SLOTS)
Review comment:
I think we are ok right now for bucket size 128 but anything larger and the `Integer` cache won't kick in here. We should consider a local cache of `Integer` routing ids and ignore any built in `Integer` cache.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
return arrayCmp(value, other.value);
}
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+ * in the same way that redis does.
+ *
+ * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+ */
+ public synchronized Object getRoutingId() {
Review comment:
Do we really want this call to be synchronized? Given that the key is immutable and we will always be asking for the routingId then why not calculate at construction and make final.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593471725
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,28 +14,44 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
+import org.apache.geode.management.ManagementException;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_REGION_BUCKETS_PARAM = "redis.region.buckets";
+ public static final int REDIS_REGION_BUCKETS =
+ Integer.getInteger(REDIS_REGION_BUCKETS_PARAM, 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
Oops - thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593466390
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +67,24 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBucketCount(int buckets) {
+ if (buckets <= 0 || ((buckets & (buckets - 1)) != 0)) {
+ throw new ManagementException(
+ String.format(
+ "Could not start Redis Server - System property '%s' must be a power of 2. Configured value is invalid: %d",
Review comment:
I'll remove the restriction - it's going to be undocumented in any case (at least for now).
----------------------------------------------------------------
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] nabarunnag commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592535527
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.executor.cluster;
+
+public class CRC16 {
+
+ // CCITT/SDLC/HDLC x^16 + x^12 + x^5 + 1 (CRC-16-CCITT)
+ private static final int CCITT_POLY = 0x8408;
+ private static final short[] crcTable = new short[256];
+
+ // Create the table up front
+ static {
+ int poly = reverseInt16(CCITT_POLY);
+
+ for (int x = 0; x < 256; x++) {
+ int w = x << 8;
+ for (int i = 0; i < 8; i++) {
+ if ((w & 0x8000) != 0) {
+ w = (w << 1) ^ poly;
+ } else {
+ w = w << 1;
+ }
+ }
+ crcTable[x] = (short) w;
+ }
+ }
+
+ /**
+ * Calculate CRC with most significant byte first. Assume all inputs are valid.
+ *
+ * @param data the byte array to use
+ * @param start starting index into the byte array
+ * @param end ending index (exclusive) into the byte array
+ */
+ public static int calculate(byte[] data, int start, int end) {
Review comment:
Still thinking, but will it be ok to keep the value as short instead of int ? To keep the algo similar to that at https://redis.io/topics/cluster-spec Appendix A
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593461293
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -39,6 +45,8 @@
*/
protected byte[] value;
+ private transient Object routingId;
Review comment:
I would hope we only call it once for each incoming key operation. So yes, calculating in the resolver would work but then we leak the internals of `ByteArrayWrapper` to another class right? What if `ByteArrayWrapper.hashCode()` was the CRC16 value, then we would be good.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593464579
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,28 +14,44 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
+import org.apache.geode.management.ManagementException;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_REGION_BUCKETS_PARAM = "redis.region.buckets";
+ public static final int REDIS_REGION_BUCKETS =
+ Integer.getInteger(REDIS_REGION_BUCKETS_PARAM, 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
Comments said you made this fixed but it is still configurable.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592705763
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.executor.cluster;
+
+public class CRC16 {
+
+ // CCITT/SDLC/HDLC x^16 + x^12 + x^5 + 1 (CRC-16-CCITT)
+ private static final int CCITT_POLY = 0x8408;
+ private static final short[] crcTable = new short[256];
+
+ // Create the table up front
+ static {
Review comment:
Since it's just statically generated, I think it's OK to leave as is.
----------------------------------------------------------------
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] dschneider-pivotal commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593352417
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +65,23 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBuckets(int buckets) {
+ if (buckets <= 0 || ((buckets & (buckets - 1)) != 0)) {
+ throw new ManagementException(
+ "Could not start Redis Server - redis region buckets must be a power of 2. Configured value is invalid: "
Review comment:
Put the system property name "redis.region.buckets" in a constant and use it in the exception strings instead of "redis region buckets" to help communicate that they set the system property to an illegal value. Some readers of this exception may not be able to tie it back to the system property otherwise.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +65,23 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBuckets(int buckets) {
Review comment:
the name "validateBuckets" is a little misleading. I thought it was going to validate the actual buckets. Perhaps "validateBucketCount" would be better.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593471560
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/cluster/CRC16JUnitTest.java
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.executor.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Test;
+
+public class CRC16JUnitTest {
Review comment:
Testing against Jedis' implementation now.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593471358
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +65,23 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBuckets(int buckets) {
+ if (buckets <= 0 || ((buckets & (buckets - 1)) != 0)) {
+ throw new ManagementException(
+ "Could not start Redis Server - redis region buckets must be a power of 2. Configured value is invalid: "
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] DonalEvans commented on pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6117:
URL: https://github.com/apache/geode/pull/6117#issuecomment-796915363
StressNewTest failures can probably be fixed by rebasing your branch on develop.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592786983
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
return arrayCmp(value, other.value);
}
+ /**
+ * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+ * in the same way that redis does.
+ *
+ * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+ */
+ public synchronized Object getRoutingId() {
Review comment:
Currently the `ByteArrayWrapper` is used for both keys and values. I have another PR that will introduce a `RedisKey` which will do exactly what you're suggesting.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787123
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
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 commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787515
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
Review comment:
Yes and 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 commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592705888
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/cluster/CRC16.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.executor.cluster;
+
+public class CRC16 {
+
+ // CCITT/SDLC/HDLC x^16 + x^12 + x^5 + 1 (CRC-16-CCITT)
+ private static final int CCITT_POLY = 0x8408;
+ private static final short[] crcTable = new short[256];
+
+ // Create the table up front
+ static {
+ int poly = reverseInt16(CCITT_POLY);
+
+ for (int x = 0; x < 256; x++) {
+ int w = x << 8;
+ for (int i = 0; i < 8; i++) {
+ if ((w & 0x8000) != 0) {
+ w = (w << 1) ^ poly;
+ } else {
+ w = w << 1;
+ }
+ }
+ crcTable[x] = (short) w;
+ }
+ }
+
+ /**
+ * Calculate CRC with most significant byte first. Assume all inputs are valid.
+ *
+ * @param data the byte array to use
+ * @param start starting index into the byte array
+ * @param end ending index (exclusive) into the byte array
+ */
+ public static int calculate(byte[] data, int start, int end) {
Review comment:
Will do.
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592787259
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
*/
package org.apache.geode.redis.internal;
+import org.apache.geode.cache.PartitionAttributesFactory;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalRegionFactory;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
public class RegionProvider {
/**
* The name of the region that holds data stored in redis.
*/
- private static final String REDIS_DATA_REGION = "__REDIS_DATA";
- private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+ public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+ public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
+ public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);
Review comment:
Made it fixed
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593443695
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -39,6 +45,8 @@
*/
protected byte[] value;
+ private transient Object routingId;
Review comment:
Do we actually call the PartitionResolver more than once on a value? I wonder if we ought to just compute the routingID in the partition resolver and not bother caching the result. This is taking up extra memory.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -51,4 +67,24 @@ public RegionProvider(InternalCache cache) {
public Region<String, Object> getConfigRegion() {
return configRegion;
}
+
+ /**
+ * Validates that the value passed in is a power of 2 and that it is not greater than
+ * {@link #REDIS_SLOTS}
+ *
+ * @throws ManagementException if there is a problem with the value
+ */
+ protected static void validateBucketCount(int buckets) {
+ if (buckets <= 0 || ((buckets & (buckets - 1)) != 0)) {
+ throw new ManagementException(
+ String.format(
+ "Could not start Redis Server - System property '%s' must be a power of 2. Configured value is invalid: %d",
Review comment:
Technically, I don't know if we really need to restrict users to powers of two. I think your math will work fine with any number of buckets.
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/cluster/CRC16JUnitTest.java
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.executor.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Test;
+
+public class CRC16JUnitTest {
Review comment:
Is this enough for us to feel confident in our implementation? Should we compare our results to Jedis or Lettuce's CRC16 implementation?
----------------------------------------------------------------
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 #6117: GEODE-9023: Add sharding support to redis region
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r593460149
##########
File path: geode-redis/src/test/java/org/apache/geode/redis/internal/executor/cluster/CRC16JUnitTest.java
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.executor.cluster;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Test;
+
+public class CRC16JUnitTest {
Review comment:
I imagine there is a sample test out there for this specific CRC16 algorithm.
----------------------------------------------------------------
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