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/05 03:31:44 UTC
[GitHub] [druid] FrankChen021 opened a new pull request #10240: Redis cache extension enhancement
FrankChen021 opened a new pull request #10240:
URL: https://github.com/apache/druid/pull/10240
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
### Description
This PR resolves #10192 .
The orignal redis cache extension is designed for standalone redis only, and does not support a couple of redis features which are widely used in production. So This PR updates the redis cache extension to:
1. add support for redis cluster
2. allow users to customize which database of a redis they want to use through new property `database`
3. add support for password protected redis servers through new property `password`
4. allow period style configuration for the existing `expiration` and `timeout` properties
### Key changes
1. orignal `RedisCache` is splitted into `AbstractRedisCache` and `RedisStandaloneCache`, the first provides a way to share common code between standalone and cluster mode redis servers
2. `RedisCacheConfig` is updated to support new properties
3. `RedisClusterCache` which inherits from `AbstractRedisCache` is added to interact with redis clusters
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added documentation for new or modified features or behaviors.
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [X] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469668015
##########
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:
SGTM
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r470380145
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
The `loadList` should be placed elsewhere in the `.spelling` file, not under the `redis-cache.md` document. Also, `loadList` is used by other document as a single word. I guess you may have missed the document `docs/configuration/index.md`. So I recommend adjusting the position of `loadList` in the `.spelling` file instead of deleting it.
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469652894
##########
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:
Yes, connection timeout and read timeout are different. But
1. the internal implementation of Jedis takes `timeout` for both connection timeout and read timeout.
2. the widely used Spring Redis also provides only one configurable timeout parameter for both connection timeout and read timeout.
So, currently I think it's acceptable to use one properties for both two parameters. I will update the doc to clarify that `timeout` property is for both connecting and reading.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] asdf2014 merged pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 merged pull request #10240:
URL: https://github.com/apache/druid/pull/10240
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469727068
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Indeed, there is no `loadList` in this document, but it still exists in other documents. We can adjust the position of `loadList` in the `.spelling` file, but it is recommended to keep it.
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469669384
##########
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:
Emm.. The maximum value returned by the `String#indexOf` method is `String#length - 1`, which makes the second judgment condition `index == host.length()` meaningless. In addition, judging by normal program logic is clearer and more efficient than judging by throwing an exception.
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469639200
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
`loadList` is not referenced in the doc, so it's useless
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r470542407
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Ah I see. I misunderstood the rules in this `.spelling` file
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469639143
##########
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:
`host` here contains the host name and port. I think the variable name is a little bit ambiguous, it should be changed to `hostAndPort` to make it clear.
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10240:
URL: https://github.com/apache/druid/pull/10240#issuecomment-669072548
CI reports that branch coverage does not meet the requirement. I'll add some more test cases later.
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469668075
##########
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:
SGTM, 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469667755
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Indeed, there is no loadList in this document, but there is still in other documents. This .spelling should be global, not specific to a particular document.
----------------------------------------------------------------
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
[GitHub] [druid] asdf2014 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469667755
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Indeed, there is no loadList in this document, but there is still in other documents. This .spelling should be global, not specific to a particular document.
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r470363197
##########
File path: website/.spelling
##########
@@ -552,7 +552,8 @@ defaultMetrics.json
namespacePrefix
src
- ../docs/development/extensions-contrib/redis-cache.md
-loadList
Review comment:
Is the `loadList` here a global configuration for all files ? I thought it was related to `redis-cache.md` doc only because it was below that file path.
BTW, I checked all files, no `loadList` is used but `druid.extensions.loadList`
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10240:
URL: https://github.com/apache/druid/pull/10240#issuecomment-673216363
Hi @asdf2014 , Thanks for your review. All the comments without my reply will be accepted, and are going to be fixed in the next commit which will come in one or two days.
----------------------------------------------------------------
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
[GitHub] [druid] FrankChen021 commented on a change in pull request #10240: Redis cache extension enhancement
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10240:
URL: https://github.com/apache/druid/pull/10240#discussion_r469644355
##########
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:
If port is not provided, the following `substring` call returns an empty string which causes the `parseInt` throw `NumberFormatException`.
There's a test case guarding this edge case whose name will be renamed to `testClusterLackOfPort` to be more clear in the following commit.
----------------------------------------------------------------
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