You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/12 15:22:40 UTC
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469200661
##########
File path: docs/development/extensions-contrib/redis-cache.md
##########
@@ -22,32 +22,85 @@ title: "Druid Redis Cache"
~ under the License.
-->
+A cache implementation for Druid based on [Redis](https://github.com/antirez/redis).
-To use this Apache Druid extension, make sure to [include](../../development/extensions.md#loading-extensions) `druid-redis-cache` extension.
+Below are guidance and configuration options known to this module.
-A cache implementation for Druid based on [Redis](https://github.com/antirez/redis).
+## Installation
+
+Use [pull-deps](../../operations/pull-deps.md) tool shipped with Druid to install this [extension](../../development/extensions.md#community-extensions) on broker, historical and middle manager nodes.
-Below are the configuration options known to this module.
+```
+java -classpath "druid_dir/lib/*" org.apache.druid.cli.Main tools pull-deps -c org.apache.druid.extensions.contrib:druid-redis-cache:{VERSION}
+```
-Note that just adding these properties does not enable the cache. You still need to add the `druid.<process-type>.cache.useCache` and `druid.<process-type>.cache.populateCache` properties for the processes you want to enable the cache on as described in the [cache configuration docs](../../configuration/index.html#cache-configuration).
+## Enabling
+
+To enable this extension after installation,
-A possible configuration would be to keep the properties below in your `common.runtime.properties` file (present on all processes) and then add `druid.<nodetype>.cache.useCache` and `druid.<nodetype>.cache.populateCache` in the `runtime.properties` file of the process types you want to enable caching on.
+1. [include](../../development/extensions.md#loading-extensions) this `druid-redis-cache` extension
+2. to enable cache on broker nodes, follow [broker caching docs](../../configuration/index.html#broker-caching) to set related properties
+3. to enable cache on historical nodes, follow [historical caching docs](../../configuration/index.html#historical-caching) to set related properties
+4. to enable cache on middle manager nodes, follow [peon caching docs](../../configuration/index.html#peon-caching) to set related properties
+5. set `druid.cache.type` to `redis`
+6. add the following properties
## Configuration
-|`common.runtime.properties`|Description|Default|Required|
+### Cluster mode
+
+To utilize a redis cluster, following properties must be set.
+
+Note: some redis cloud service providers provide redis cluster service via a redis proxy, for these clusters, please follow the [Standalone mode](#standalone-mode) configuration below.
+
+| Properties |Description|Default|Required|
+|--------------------|-----------|-------|--------|
+|`druid.cache.cluster.nodes`| Redis nodes in a cluster, represented in comma separated string. See example below | None | yes |
+|`druid.cache.cluster.maxRedirection`| Max retry count | 5 | None |
Review comment:
The fourth column `Required` should be filled with `yes` or `no`, not `None`.
##########
File path: docs/development/extensions-contrib/redis-cache.md
##########
@@ -22,32 +22,85 @@ title: "Druid Redis Cache"
~ under the License.
-->
+A cache implementation for Druid based on [Redis](https://github.com/antirez/redis).
-To use this Apache Druid extension, make sure to [include](../../development/extensions.md#loading-extensions) `druid-redis-cache` extension.
+Below are guidance and configuration options known to this module.
-A cache implementation for Druid based on [Redis](https://github.com/antirez/redis).
+## Installation
+
+Use [pull-deps](../../operations/pull-deps.md) tool shipped with Druid to install this [extension](../../development/extensions.md#community-extensions) on broker, historical and middle manager nodes.
-Below are the configuration options known to this module.
+```
Review comment:
Please specify a bash type for this block
##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheFactory.java
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.druid.client.cache;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.druid.java.util.common.IAE;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.JedisPool;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class RedisCacheFactory
+{
+ public static Cache create(final RedisCacheConfig config)
+ {
+ if (config.getCluster() != null && StringUtils.isNotBlank(config.getCluster().getNodes())) {
+
+ Set<HostAndPort> nodes = Arrays.stream(config.getCluster().getNodes().split(","))
+ .map(String::trim)
+ .filter(StringUtils::isNotBlank)
+ .map(host -> {
+ int index = host.indexOf(':');
+ if (index <= 0 || index == host.length()) {
Review comment:
The second judgment should be to avoid the lack of port. For example, `127.0.0.1:`. I think it should be changed to `index == host.length() - 1` instead of `index == host.length()`.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
Review comment:
This instance of `MockJedisPool` is useless, please delete it.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
+ // orginal MockJedis do not support 'milliseconds' in long type,
+ // for test we override to support it
+ cache = new RedisClusterCache(new MockJedisCluster(Sets.newHashSet(new HostAndPort("localhost", 6379)))
Review comment:
Emm.. Generally, we don't recommend using `Sets.newHashSet` to create `HashSet` instances, and we hope to create them directly through new. In addition, it may be more appropriate to use `Collections.singleton` here.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
+ // orginal MockJedis do not support 'milliseconds' in long type,
+ // for test we override to support it
+ cache = new RedisClusterCache(new MockJedisCluster(Sets.newHashSet(new HostAndPort("localhost", 6379)))
+ {
+ Map<String, byte[]> cacheStorage = new HashMap<>();
+
+ @Override
+ public String setex(final byte[] key, final int seconds, final byte[] value)
+ {
+ cacheStorage.put(Base64.getEncoder().encodeToString(key), value);
Review comment:
Let's use the existing tool method here: `StringUtils.encodeBase64String`.
##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheConfig.java
##########
@@ -83,4 +178,19 @@ public int getMinIdleConnections()
{
return minIdleConnections;
}
+
+ public RedisClusterConfig getCluster()
+ {
+ return cluster;
+ }
+
+ public String getPassword()
Review comment:
We need to pay attention to security and avoid exposing password information. For example, we can consider adding `@JsonIgnore` annotation.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
Review comment:
Let's write it as `"PT2S"`.
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Is it really necessary to delete `loadList`?
##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisClusterCache.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.druid.client.cache;
+
+import redis.clients.jedis.JedisCluster;
+
+import java.io.IOException;
+import java.util.List;
+
+public class RedisClusterCache extends AbstractRedisCache
+{
+ private JedisCluster cluster;
+
+ RedisClusterCache(JedisCluster cluster, RedisCacheConfig config)
+ {
+ super(config);
+ this.cluster = cluster;
+ }
+
+ @Override
+ protected byte[] getFromRedis(byte[] key)
+ {
+ return cluster.get(key);
+ }
+
+ @Override
+ protected void putToRedis(byte[] key, byte[] value, RedisCacheConfig.DurationConfig expiration)
+ {
+ cluster.setex(key, (int) expiration.getSeconds(), value);
+ }
+
+ @Override
+ protected List<byte[]> mgetFromRedis(byte[]... keys)
+ {
+ return cluster.mget(keys);
+ }
+
+ @Override
+ protected void cleanup()
+ {
+ try {
+ cluster.close();
+ }
+ catch (IOException e) {
Review comment:
Please rename this `e` to `ignored`.
##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheFactory.java
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.druid.client.cache;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.druid.java.util.common.IAE;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.JedisPool;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class RedisCacheFactory
+{
+ public static Cache create(final RedisCacheConfig config)
+ {
+ if (config.getCluster() != null && StringUtils.isNotBlank(config.getCluster().getNodes())) {
+
+ Set<HostAndPort> nodes = Arrays.stream(config.getCluster().getNodes().split(","))
+ .map(String::trim)
+ .filter(StringUtils::isNotBlank)
+ .map(host -> {
+ int index = host.indexOf(':');
+ if (index <= 0 || index == host.length()) {
+ throw new IAE("Invalid redis cluster configuration: %s", host);
+ }
+
+ int port;
+ try {
+ port = Integer.parseInt(host.substring(index + 1));
+ }
+ catch (NumberFormatException e) {
+ throw new IAE("Invalid redis cluster configuration: invalid port %s", host);
+ }
+ if (port <= 0 || port > 65535) {
+ throw new IAE("Invalid redis cluster configuration: invalid port %s", host);
+ }
+
+ return new HostAndPort(host.substring(0, index), port);
+ }).collect(Collectors.toSet());
+
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(config.getMaxTotalConnections());
+ poolConfig.setMaxIdle(config.getMaxIdleConnections());
+ poolConfig.setMinIdle(config.getMinIdleConnections());
+
+ JedisCluster cluster;
+ if (StringUtils.isNotBlank(config.getPassword())) {
+ cluster = new JedisCluster(
+ nodes,
+ config.getTimeout().getMillisecondsAsInt(),
+ config.getTimeout().getMillisecondsAsInt(),
Review comment:
There is still a difference between connectionTimeout and soTimeout. If possible, please extend the soTimeout parameter.
##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisCacheFactory.java
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.druid.client.cache;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.druid.java.util.common.IAE;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.JedisPool;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class RedisCacheFactory
+{
+ public static Cache create(final RedisCacheConfig config)
+ {
+ if (config.getCluster() != null && StringUtils.isNotBlank(config.getCluster().getNodes())) {
+
+ Set<HostAndPort> nodes = Arrays.stream(config.getCluster().getNodes().split(","))
+ .map(String::trim)
+ .filter(StringUtils::isNotBlank)
+ .map(host -> {
+ int index = host.indexOf(':');
+ if (index <= 0 || index == host.length()) {
+ throw new IAE("Invalid redis cluster configuration: %s", host);
+ }
+
+ int port;
+ try {
+ port = Integer.parseInt(host.substring(index + 1));
+ }
+ catch (NumberFormatException e) {
+ throw new IAE("Invalid redis cluster configuration: invalid port %s", host);
+ }
+ if (port <= 0 || port > 65535) {
+ throw new IAE("Invalid redis cluster configuration: invalid port %s", host);
Review comment:
I believe that `port` should be passed in instead of `host`.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
##########
@@ -82,20 +83,25 @@ public String psetex(byte[] key, long milliseconds, byte[] value)
}
});
- cache = RedisCache.create(pool, cacheConfig);
+ cache = new RedisStandaloneCache(pool, cacheConfig);
}
@Test
public void testBasicInjection() throws Exception
{
- final RedisCacheConfig config = new RedisCacheConfig();
+ String json = "{ \"host\": \"localhost\", \"port\":6379, \"expiration\": 3600}";
Review comment:
nit: Adding a space in the middle of `\"port\":6379` will satisfy the JSON formatting requirements.
##########
File path: docs/development/extensions-contrib/redis-cache.md
##########
@@ -22,32 +22,85 @@ title: "Druid Redis Cache"
~ under the License.
-->
+A cache implementation for Druid based on [Redis](https://github.com/antirez/redis).
Review comment:
Please update the address to [https://github.com/redis/redis](https://github.com/redis/redis) by the way.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
+ // orginal MockJedis do not support 'milliseconds' in long type,
+ // for test we override to support it
+ cache = new RedisClusterCache(new MockJedisCluster(Sets.newHashSet(new HostAndPort("localhost", 6379)))
Review comment:
FYI, https://github.com/apache/druid/blob/master/codestyle/druid-forbidden-apis.txt#L19
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
+ // orginal MockJedis do not support 'milliseconds' in long type,
+ // for test we override to support it
+ cache = new RedisClusterCache(new MockJedisCluster(Sets.newHashSet(new HostAndPort("localhost", 6379)))
+ {
+ Map<String, byte[]> cacheStorage = new HashMap<>();
+
+ @Override
+ public String setex(final byte[] key, final int seconds, final byte[] value)
+ {
+ cacheStorage.put(Base64.getEncoder().encodeToString(key), value);
+ return null;
+ }
+
+ @Override
+ public byte[] get(final byte[] key)
+ {
+ return cacheStorage.get(Base64.getEncoder().encodeToString(key));
Review comment:
Let's use the existing tool method here: `StringUtils.encodeBase64String`.
##########
File path: extensions-contrib/redis-cache/pom.xml
##########
@@ -78,6 +78,36 @@
<artifactId>jackson-databind</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-compress</artifactId>
+ <version>1.19</version>
Review comment:
The version here does not need to be specified, it will be automatically inherited from the parent module. The others are the same.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.druid.client.cache;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fiftyonred.mock_jedis.MockJedisCluster;
+import com.fiftyonred.mock_jedis.MockJedisPool;
+import com.google.common.collect.Lists;
+import com.google.common.primitives.Ints;
+import org.apache.commons.compress.utils.Sets;
+import org.apache.druid.java.util.common.StringUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisPoolConfig;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class RedisClusterCacheTest
+{
+ private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
+ private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
+
+ private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
+ {
+ @Override
+ public DurationConfig getTimeout()
+ {
+ return new DurationConfig(2000);
+ }
+
+ @Override
+ public DurationConfig getExpiration()
+ {
+ return new DurationConfig("PT1H");
+ }
+ };
+
+ private RedisClusterCache cache;
+
+ @Before
+ public void setUp()
+ {
+ JedisPoolConfig poolConfig = new JedisPoolConfig();
+ poolConfig.setMaxTotal(cacheConfig.getMaxTotalConnections());
+ poolConfig.setMaxIdle(cacheConfig.getMaxIdleConnections());
+ poolConfig.setMinIdle(cacheConfig.getMinIdleConnections());
+
+ MockJedisPool pool = new MockJedisPool(poolConfig, "localhost");
+ // orginal MockJedis do not support 'milliseconds' in long type,
+ // for test we override to support it
+ cache = new RedisClusterCache(new MockJedisCluster(Sets.newHashSet(new HostAndPort("localhost", 6379)))
+ {
+ Map<String, byte[]> cacheStorage = new HashMap<>();
+
+ @Override
+ public String setex(final byte[] key, final int seconds, final byte[] value)
+ {
+ cacheStorage.put(Base64.getEncoder().encodeToString(key), value);
+ return null;
+ }
+
+ @Override
+ public byte[] get(final byte[] key)
+ {
+ return cacheStorage.get(Base64.getEncoder().encodeToString(key));
+ }
+
+ @Override
+ public List<byte[]> mget(final byte[]... keys)
+ {
+ List<byte[]> ret = new ArrayList<>();
+ for (byte[] key : keys) {
+ String k = Base64.getEncoder().encodeToString(key);
Review comment:
Let's use the existing tool method here: `StringUtils.encodeBase64String`.
##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisStandaloneCacheTest.java
##########
@@ -41,24 +42,24 @@
import java.util.Map;
import java.util.UUID;
-public class RedisCacheTest
+public class RedisStandaloneCacheTest
{
private static final byte[] HI = StringUtils.toUtf8("hiiiiiiiiiiiiiiiiiii");
private static final byte[] HO = StringUtils.toUtf8("hooooooooooooooooooo");
- private RedisCache cache;
+ private RedisStandaloneCache cache;
private final RedisCacheConfig cacheConfig = new RedisCacheConfig()
{
@Override
- public int getTimeout()
+ public DurationConfig getTimeout()
{
- return 10;
+ return new DurationConfig(2000);
Review comment:
Let's write it as `"PT2S"`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org