membphis commented on PR #13579: URL: https://github.com/apache/apisix/pull/13579#issuecomment-4764463557
Blocking concern: this dependency upgrade appears to change Redis Cluster timeout/retry behavior implicitly, beyond the intended `NOSCRIPT` fix. The `NOSCRIPT` handling in `lua-resty-redis-cluster = 1.3.3-0` matches the `limit-count` `evalsha -> eval` fallback, so that part looks good. However, the replacement is not behavior-neutral compared with `resty-redis-cluster = 1.05-1`: - The old library defaults to `DEFAULT_MAX_REDIRECTION = 5` and `DEFAULT_MAX_CONNECTION_ATTEMPTS = 3`; the new library defaults to `2` and `2`. - APISIX currently passes `connect_timeout/read_timeout/send_timeout = conf.redis_timeout` from `apisix/utils/rediscluster.lua`, and the plugin docs/schema expose `redis_timeout` as applying to `redis-cluster`. - In the new library, `try_hosts_slots()` and `handle_command_with_retry()` use fixed connect timeouts (`100ms` for the first attempt, `800ms` for later attempts) instead of consistently using `config.connect_timeout`. This can make `policy = "redis-cluster"` fail earlier than before in high-latency, TLS, cross-AZ, rolling replacement, MOVED/ASK-heavy, or transient node-failure scenarios. More importantly, increasing `redis_timeout` may no longer fully affect the Redis Cluster connection path as documented. Could we either preserve the previous APISIX behavior in the wrapper, for example by explicitly setting the old retry defaults and ensuring `redis_timeout` is still honored by the cluster connect paths, or document the compatibility change and add tests that cover the new timeout/retry semantics? Without that, this dependency upgrade has a user-visible compatibility risk that is not disclosed by the PR title/description. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
