Add ParseURL function for cluster mode

[stephaniehingtgen]

What this PR does
This PR adds a new function, ParseClusterURL, which takes a string and turns it into the array of addresses used for redis in cluster mode along with configurations passed in.

How it works
It is very similar to the ParseURL function, except for this function only allows for the redis and rediss schemes and will take one to many addr parameters. It will get the username and password for the cluster as well as configurations sent in through the query parameters.

Examples that will succeed:

"rediss://foo:bar@localhost:6378?read_timeout=1s&addr=localhost:6379"
"redis://foo:bar@localhost:6378?read_timeout=1s&addr=localhost:6379&addr=localhost:6380"

Examples that will fail:

"redis://foo:bar@localhost:6378?read_timeout=1s&addr=localhost:6379&addr=redis://localhost:6380" // cannot use scheme again in params
"localhost:6378?read_timeout=1s&addr=localhost:6379&addr=localhost:6380" // must use redis or rediss scheme

[vmihailenco]

Thanks for the PR and the detailed comment 👍

only the last value is interpreted if a parameter is given multiple times

This is kinda strange, especially since you can connect to a Redis Cluster using a single address. Have you considered using something lke redis://foo:bar@localhost:6378?read_timeout=1s&addr=localhost:6379&addr=localhost:6380?

And it is unfortunate that we need to duplicate the code for Client and ClusterClient, but I don't see an easy way around this...

---

I would also consider using options API, for example:

redis.NewClient(&redis.Options{}, redis.WithDSN("..."))
redis.NewClusterClient(&redis.ClusterOptions{}, redis.WithDSN("..."))

Then redis.Options and redis.ClusterOptions could implement a private interface, e.g.

type configurator interface {
    setTimeout(d time.Duration) // redis.WithTimeout(d)
    setDB(db int) // redis.WithDB(db)
    setValues(query *queryOptions) // unrecognized options specific to the client
}

var _ configurator = (*Options)(nil)
var _ configurator = (*ClusterOptions)(nil)

func WithTimeout(d time.Duration) redis.Option {
    return func(c configurator) {
        c.setTimeout(d)
    }
}

This is a much bigger change, but it looks much nicer and more flexible than the exiting one.

[stephaniehingtgen]

Thanks for the PR and the detailed comment 👍

only the last value is interpreted if a parameter is given multiple times

This is kinda strange, especially since you can connect to a Redis Cluster using a single address. Have you considered using something lke redis://foo:bar@localhost:6378?read_timeout=1s&addr=localhost:6379&addr=localhost:6380?

That's a great idea - thanks! I updated the PR to take in one string and be able to read as many addr params as specified. I'll update the description of the PR to match what the PR is currently doing.

I would also consider using options API, for example:

redis.NewClient(&redis.Options{}, redis.WithDSN("..."))
redis.NewClusterClient(&redis.ClusterOptions{}, redis.WithDSN("..."))

Then redis.Options and redis.ClusterOptions could implement a private interface, e.g.

type configurator interface {
    setTimeout(d time.Duration) // redis.WithTimeout(d)
    setDB(db int) // redis.WithDB(db)
    setValues(query *queryOptions) // unrecognized options specific to the client
}

var _ configurator = (*Options)(nil)
var _ configurator = (*ClusterOptions)(nil)

func WithTimeout(d time.Duration) redis.Option {
    return func(c configurator) {
        c.setTimeout(d)
    }
}

This is a much bigger change, but it looks much nicer and more flexible than the exiting one.

I started looking into this, and it started to seem like it would be a fairly large refactor (as the timeouts are in the base client). Also, the cluster client doesn't have a notion of DB - not sure if this impacts what you're thinking for this. Since this would be a refactor rather than a feature, I think it would be best to have that work done apart from the PR. Is that okay with you?

[vmihailenco]

I think it would be best to have that work done apart from the PR. Is that okay with you?

That is okay, but ideally we want to support a single way to connect to a Redis Cluster via NewClusterClient without introducing ParseClusterClient.

(as the timeouts are in the base client)

It should just set opt.ReadTimeout, opt.WriteTimeout, and opt.DialTimeout. I did not mean to use Client.WithTimeout.

the cluster client doesn't have a notion of DB - not sure if this impacts what you're thinking for this

setDB(db int) could return an error saying that DB option is not supported.

[vmihailenco]

For what it is worth it, I've "documented" this idea at https://blog.uptrace.dev/posts/go-functional-options-named-args.html#reusing-options-for-different-purposes

[stephaniehingtgen]

👋 Hey, @vmihailenco, just wanted to follow up and see if you had any further comments on this PR?

[vmihailenco]

@stephaniehingtgen this looks good, but I need some time to experiment with options API first before we merge this PR and make it available for everyone. Sorry for the delay and thanks for your work 👍

[scotchneat]

Excited to see this.

One comment regarding the URL format was that I would have expected the list of hosts to be a comma-delimited list in the host part of the URL (like a mongo uri)

rediss://user:pass@host1:6379,host2:6380,host3:6381?sslmode=verify-full&connect_timeout=10

This would allow the query parameters to map to connection configuration options.

[jrots]

Great stuff, any chance that this could get merged? In need of TLS support when connecting to an elisticache redis cluster here.. 😇

[stephaniehingtgen]

Excited to see this.

One comment regarding the URL format was that I would have expected the list of hosts to be a comma-delimited list in the host part of the URL (like a mongo uri)

rediss://user:pass@host1:6379,host2:6380,host3:6381?sslmode=verify-full&connect_timeout=10

This would allow the query parameters to map to connection configuration options.

I'd be happy to make this change! I'd just like to wait to get the go ahead from @vmihailenco before continuing work on this PR. If this PR is going to be used, then I will make that change asap :)