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 2022/10/06 18:12:06 UTC
[GitHub] [bahir-flink] srmrz opened a new pull request, #156: Bump redis version to 4.2.9
srmrz opened a new pull request, #156:
URL: https://github.com/apache/bahir-flink/pull/156
Trying to expedite this change
https://github.com/apache/bahir-flink/pull/155
Splitting #155 into two separate changes.
cc @eskabetxe
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] srmrz commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
srmrz commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991403682
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
re:override, sure, let me do that!
I'll take a look at the "communication test"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] srmrz closed pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
srmrz closed pull request #156: Bump redis version to 4.2.9
URL: https://github.com/apache/bahir-flink/pull/156
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] srmrz commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
srmrz commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r995758210
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
Checking in on this. Let me know if there are other questions
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] srmrz commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
srmrz commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991500442
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
@eskabetxe I see!
> I don't know if is needed
My take is that it isn't needed - the constructor takes a `JedisCluster`.
As part of the initialization of `JedisCluster` , it contacts the cluster:
https://github.com/redis/jedis/blob/379bbed0ee363bfc9f31e4591f5d46c28976cff5/src/main/java/redis/clients/jedis/UnifiedJedis.java#L105-L107
https://github.com/redis/jedis/blob/379bbed0ee363bfc9f31e4591f5d46c28976cff5/src/main/java/redis/clients/jedis/providers/ClusterConnectionProvider.java#L27
https://github.com/redis/jedis/blob/379bbed0ee363bfc9f31e4591f5d46c28976cff5/src/main/java/redis/clients/jedis/providers/ClusterConnectionProvider.java#L36-L49
That runs commands in the cluster. So if there are problems contacting the cluster, we'll know before we even construct a `RedisClusterContainer`
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] eskabetxe commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
eskabetxe commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991455017
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
@srmrz "jedisCluster.echo("Test");"
I don't know if is needed.. but for the comment its testing the communication with cluster..
I don't write this code, I'm trying to understand if removing that code have some side effect..
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] eskabetxe commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
eskabetxe commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991223392
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
Could you leave de override?
the "communication test" is not needed anymore?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] srmrz commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
srmrz commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991408042
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
@eskabetxe, added override here [94e2078](https://github.com/apache/bahir-flink/pull/156/commits/94e2078feec7b98d22a083b23c9a826a836c6d80)
Mind pointing me at the communication test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bahir-flink] eskabetxe commented on a diff in pull request #156: Bump redis version to 4.2.9
Posted by GitBox <gi...@apache.org>.
eskabetxe commented on code in PR #156:
URL: https://github.com/apache/bahir-flink/pull/156#discussion_r991455017
##########
flink-connector-redis/src/main/java/org/apache/flink/streaming/connectors/redis/common/container/RedisClusterContainer.java:
##########
@@ -46,15 +46,7 @@ public RedisClusterContainer(JedisCluster jedisCluster) {
this.jedisCluster = jedisCluster;
}
- @Override
- public void open() throws Exception {
-
- // echo() tries to open a connection and echos back the
- // message passed as argument. Here we use it to monitor
- // if we can communicate with the cluster.
-
- jedisCluster.echo("Test");
- }
+ public void open() {}
Review Comment:
@srmrz "jedisCluster.echo("Test");"
I don't know if is needed.. but for the comment its testing the communication with cluster..
I don't write this code, I'm trying to understand if removing that code could have some side effect..
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@bahir.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org