You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "vttranlina (via GitHub)" <gi...@apache.org> on 2025/04/28 04:27:54 UTC
[PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
vttranlina opened a new pull request, #2706:
URL: https://github.com/apache/james-project/pull/2706
Currently, in the `RedisHealthcheck`, we are using Reactor's timeout handling, not the timeout provided by Redis Lettuce.
With this configuration, the timeout will be correctly handled by the Lettuce library
Reference: https://github.com/redis/lettuce/discussions/2614
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2706:
URL: https://github.com/apache/james-project/pull/2706#discussion_r2085882121
##########
backends-common/redis/src/test/java/org/apache/james/backends/redis/RedisSentinelExtension.java:
##########
@@ -109,7 +109,7 @@ private String createRedisSentinelURI() {
sb.append("redis-sentinel://123@");
sb.append(this.stream().map(container -> container.getHost() + ":" + container.getMappedPort(SENTINEL_PORT))
.collect(Collectors.joining(",")));
- sb.append("?sentinelMasterId=mymaster");
+ sb.append("?sentinelMasterId=mymaster&timeout=3s");
Review Comment:
I think we can keep removing reactor timeout.
As we have healthcheck timeout safeguard already https://github.com/apache/james-project/blob/11646b05bbc539492fd3715230c22388174a30ad/server/container/guice/common/src/main/java/org/apache/james/PeriodicalHealthChecks.java#L59.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
Posted by "vttranlina (via GitHub)" <gi...@apache.org>.
vttranlina commented on code in PR #2706:
URL: https://github.com/apache/james-project/pull/2706#discussion_r2065380004
##########
backends-common/redis/src/test/java/org/apache/james/backends/redis/RedisSentinelExtension.java:
##########
@@ -109,7 +109,7 @@ private String createRedisSentinelURI() {
sb.append("redis-sentinel://123@");
sb.append(this.stream().map(container -> container.getHost() + ":" + container.getMappedPort(SENTINEL_PORT))
.collect(Collectors.joining(",")));
- sb.append("?sentinelMasterId=mymaster");
+ sb.append("?sentinelMasterId=mymaster&timeout=3s");
Review Comment:
I'm no problem with both way
feel free to revert 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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2706:
URL: https://github.com/apache/james-project/pull/2706#discussion_r2065318285
##########
backends-common/redis/src/test/java/org/apache/james/backends/redis/RedisSentinelExtension.java:
##########
@@ -109,7 +109,7 @@ private String createRedisSentinelURI() {
sb.append("redis-sentinel://123@");
sb.append(this.stream().map(container -> container.getHost() + ":" + container.getMappedPort(SENTINEL_PORT))
.collect(Collectors.joining(",")));
- sb.append("?sentinelMasterId=mymaster");
+ sb.append("?sentinelMasterId=mymaster&timeout=3s");
Review Comment:
This requires the extra configuration change (not everyone is aware of it and applies it everywhere)
I would be defensive in keeping the Reactor timeout as the safeguard.
WDYT?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2706:
URL: https://github.com/apache/james-project/pull/2706#discussion_r2072912080
##########
backends-common/redis/src/test/java/org/apache/james/backends/redis/RedisSentinelExtension.java:
##########
@@ -109,7 +109,7 @@ private String createRedisSentinelURI() {
sb.append("redis-sentinel://123@");
sb.append(this.stream().map(container -> container.getHost() + ":" + container.getMappedPort(SENTINEL_PORT))
.collect(Collectors.joining(",")));
- sb.append("?sentinelMasterId=mymaster");
+ sb.append("?sentinelMasterId=mymaster&timeout=3s");
Review Comment:
Change or not change? Status on this?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] JAMES-4072 Enable timeout option in RedisClientFactory [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 merged PR #2706:
URL: https://github.com/apache/james-project/pull/2706
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org