[PR] [fix][io] Reject multiple redisHosts in Redis sink Standalone mode [pulsar-connectors]

["david-streamlio (via GitHub)" <gi...@apache.org>]

david-streamlio opened a new pull request, #143:
URL: https://github.com/apache/pulsar-connectors/pull/143

   Fixes #141
   
   ### Motivation
   
   In `Standalone` mode, `RedisSession.create()` builds a `RedisURI` for every entry in `redisHosts` and then connects only to `redisURIs.get(0)`. The other hosts are dropped without a warning, so a user who lists several hosts for redundancy, or who forgets to set `clientMode: Cluster`, gets a single-node connection with no sign that the rest of the list was ignored.
   
   ### Modifications
   
   - `RedisAbstractConfig.validate()` rejects a `redisHosts` value with more than one host when `clientMode` is `Standalone` (case-insensitive, which matches how `RedisSession` parses the mode). The error names the configured hosts and suggests `clientMode: Cluster`.
   - The `redisHosts` `@FieldDoc` now says Standalone takes exactly one host and Cluster uses the list as seed nodes.
   
   **Compatibility note:** a deployment that currently lists several hosts in Standalone mode will now fail at startup instead of quietly connecting to the first host. That is the intent (the issue's preferred option), but it is a visible change on upgrade. The fix is to keep a single host or switch to `clientMode: Cluster`.
   
   ### Verifying this change
   
   New `RedisSinkConfigTest` cases:
   - `standaloneWithMultipleHostsFailsValidateTest`: two hosts with `clientMode: Standalone` fails validation.
   - `defaultClientModeWithMultipleHostsFailsValidateTest`: two hosts with the default `clientMode` fails validation.
   - `clusterWithMultipleHostsValidateTest`: two hosts with `clientMode: Cluster` still passes.
   
   `./gradlew :redis:test --tests '*RedisSinkConfigTest' --tests '*RedisSessionTest'` passes (37 tests), and `:redis:check -x test` passes.
   
   ### Documentation
   
   - [ ] `doc`
   - [x] `doc-required` (the pulsar-site Redis sink page should mention the single-host rule for Standalone mode)
   - [ ] `doc-not-needed`
   - [ ] `doc-complete`
   
   🤖 Generated with [Claude Code](https://claude.com/claude-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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][io] Reject multiple redisHosts in Redis sink Standalone mode [pulsar-connectors]

["david-streamlio (via GitHub)" <gi...@apache.org>]

david-streamlio merged PR #143:
URL: https://github.com/apache/pulsar-connectors/pull/143


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][io] Reject multiple redisHosts in Redis sink Standalone mode [pulsar-connectors]

["david-streamlio (via GitHub)" <gi...@apache.org>]

david-streamlio commented on PR #143:
URL: https://github.com/apache/pulsar-connectors/pull/143#issuecomment-6255487332

   @kritharth2005, since you worked on the Redis ACL/TLS changes in #126 and #135, would you mind taking a look at this one too? It adds a Standalone-mode check to `RedisAbstractConfig.validate()`. #144 touches the same method for the Cluster-mode `redisDatabase` check.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][io] Reject multiple redisHosts in Redis sink Standalone mode [pulsar-connectors]

["kritharth2005 (via GitHub)" <gi...@apache.org>]

kritharth2005 commented on code in PR #143:
URL: https://github.com/apache/pulsar-connectors/pull/143#discussion_r4317415386


##########
redis/src/main/java/org/apache/pulsar/io/redis/RedisAbstractConfig.java:
##########
@@ -143,6 +144,10 @@ public class RedisAbstractConfig implements Serializable {
 
     public void validate() {
         Preconditions.checkNotNull(clientMode, "clientMode property not set.");
+        Preconditions.checkArgument(!ClientMode.STANDALONE.name().equalsIgnoreCase(clientMode)

Review Comment:
   Edge case: `StringUtils.split` drops empty tokens but not whitespace-only ones, so `redisHosts: "localhost:6379, "` produces a `" "` token and counts as two hosts. I checked this locally: that config fails here with “lists several: localhost:6379, .” even though it names one host, and before this change it connected fine.
   
   Could be handled by skipping blank tokens, either in this check or in `getHostAndPorts()` (the latter would also stop a `" "` seed URI in Cluster mode). Happy either way if you’d rather keep this PR minimal.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org