You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@bahir.apache.org by GitBox <gi...@apache.org> on 2020/12/16 09:21:02 UTC
[GitHub] [bahir-flink] ahern88 opened a new pull request #101: support set redis cluster password
ahern88 opened a new pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101
Now redis cluster sink setting password is not supported
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] ahern88 commented on a change in pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
ahern88 commented on a change in pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#discussion_r558203282
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/config/handler/FlinkJedisClusterConfigHandler.java
##########
@@ -44,8 +45,16 @@ public FlinkJedisConfigBase createFlinkJedisConfig(Map<String, String> propertie
String[] arr = r.split(":");
return new InetSocketAddress(arr[0].trim(), Integer.parseInt(arr[1].trim()));
}).collect(Collectors.toSet());
- return new FlinkJedisClusterConfig.Builder()
- .setNodes(nodes).build();
+ String clusterPassword = properties.getOrDefault(REDIS_CLUSTER_PASSWORD, null);
+ if (clusterPassword != null && clusterPassword.trim().isEmpty()) {
Review comment:
Thanks, i optimized the code
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] eskabetxe commented on a change in pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
eskabetxe commented on a change in pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#discussion_r557994920
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/config/handler/FlinkJedisClusterConfigHandler.java
##########
@@ -44,8 +45,16 @@ public FlinkJedisConfigBase createFlinkJedisConfig(Map<String, String> propertie
String[] arr = r.split(":");
return new InetSocketAddress(arr[0].trim(), Integer.parseInt(arr[1].trim()));
}).collect(Collectors.toSet());
- return new FlinkJedisClusterConfig.Builder()
- .setNodes(nodes).build();
+ String clusterPassword = properties.getOrDefault(REDIS_CLUSTER_PASSWORD, null);
+ if (clusterPassword != null && clusterPassword.trim().isEmpty()) {
Review comment:
this if could be merged with the if on line 54 no?
if (clusterPassword != null && clusterPassword.trim().isEmpty()) {
builder.setPassword(clusterPassword);
}
or directly
if (StringUtils.isNotBlank(clusterPassword))
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] ahern88 edited a comment on pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
ahern88 edited a comment on pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#issuecomment-760310045
Ok, I have optimized it out, Please help to check, thanks @lresende
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] eskabetxe merged pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
eskabetxe merged pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] lresende commented on a change in pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
lresende commented on a change in pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#discussion_r554804283
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/RedisTableSinkFactory.java
##########
@@ -57,6 +57,7 @@
properties.add(REDIS_COMMAND);
properties.add(REDIS_NODES);
properties.add(REDIS_MASTER_NAME);
+ properties.add(CLUSTER_PASSWORD);
Review comment:
Could we follow the same pattern for the variables (e.g. `REDIS_CLUSTER_PASSWORD`)
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/config/handler/FlinkJedisClusterConfigHandler.java
##########
@@ -16,10 +16,6 @@
*/
package org.apache.flink.streaming.connectors.redis.common.config.handler;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_CLUSTER;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_MODE;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_NODES;
-
Review comment:
Could we please continue using explicit imports?
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/descriptor/RedisValidator.java
##########
@@ -30,6 +30,7 @@
public static final String REDIS_MASTER_NAME = "master.name";
public static final String SENTINELS_INFO = "sentinels.info";
public static final String SENTINELS_PASSWORD = "sentinels.password";
+ public static final String CLUSTER_PASSWORD = "cluster.password";
Review comment:
Could we follow the same pattern for the variables (e.g. `REDIS_CLUSTER_PASSWORD`)
##########
File path: flink-connector-redis/src/test/java/org/apache/flink/streaming/connectors/redis/common/RedisHandlerTest.java
##########
@@ -17,13 +17,8 @@
package org.apache.flink.streaming.connectors.redis.common;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_CLUSTER;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_COMMAND;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_KEY_TTL;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_MODE;
-import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.REDIS_NODES;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.*;
+import static org.junit.Assert.*;
Review comment:
Could we please continue using explicit imports?
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/config/handler/FlinkJedisClusterConfigHandler.java
##########
@@ -31,6 +27,8 @@
import org.apache.flink.streaming.connectors.redis.common.hanlder.FlinkJedisConfigHandler;
import org.apache.flink.util.Preconditions;
+import static org.apache.flink.streaming.connectors.redis.descriptor.RedisValidator.*;
Review comment:
see above
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] ahern88 commented on pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
ahern88 commented on pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#issuecomment-760310045
Ok, I have optimized it out, Please help to check, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] ahern88 commented on a change in pull request #101: support set redis cluster password
Posted by GitBox <gi...@apache.org>.
ahern88 commented on a change in pull request #101:
URL: https://github.com/apache/bahir-flink/pull/101#discussion_r558205096
##########
File path: flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/config/handler/FlinkJedisClusterConfigHandler.java
##########
@@ -44,8 +45,16 @@ public FlinkJedisConfigBase createFlinkJedisConfig(Map<String, String> propertie
String[] arr = r.split(":");
return new InetSocketAddress(arr[0].trim(), Integer.parseInt(arr[1].trim()));
}).collect(Collectors.toSet());
- return new FlinkJedisClusterConfig.Builder()
- .setNodes(nodes).build();
+ String clusterPassword = properties.getOrDefault(REDIS_CLUSTER_PASSWORD, null);
+ if (clusterPassword != null && clusterPassword.trim().isEmpty()) {
Review comment:
Thanks, i optimized the code
if (StringUtils.isNotBlank(clusterPassword)) {
builder.setPassword(clusterPassword);
}
----------------------------------------------------------------
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