mediocregopher / radix.v2

Redis client for Go
http://godoc.org/github.com/mediocregopher/radix.v2
MIT License
433 stars 92 forks source link

Initial sentinel dial has no timeout #68

Open fillest opened 7 years ago

fillest commented 7 years ago

sentinel.NewClientCustom uses hardcoded redis.Dial at the beginning. Shouldn't it use the same passed df DialFunc instead? So one could pass a DialTimeout wrapper.

mediocregopher commented 7 years ago

Hi @fillest ! thanks for submitting the issue. The df DialFunc is defined as being for the connections being made to the redis master, not the sentinel instance itself. I'd like to keep it separated as people use the DialFunc for more than just timeouts (e.g. it can be used for AUTH commands as well), so the DialFunc for the actual redis instances might not be appropriate for the sentinel.

I've had a lot of issues submitted lately about the sentinel package though, it wasn't designed particularly well when I first did so, and I'm thinking I'll add a sentinel.NewClientWithOpts function which can take in an options struct (like in cluster.NewWithOpts) and use that to address a lot of the feedback. I'll definitely keep this issue in mind when I do so, and I'll reference this issue number in the commit so you'll get a notification.

fillest commented 7 years ago

Hi. Ah, yeah, auth.. OK, thanks. In the meantime I'm wrapping it with:

func DoWithTimeout(cb func(), t time.Duration, panicValue interface{}) {
    done := make(chan bool, 1)
    go func() {
        defer someInternalPanicReporter()
        cb()
        done <- true
    }()
    select {
    case <-done:
    case <-time.After(t):
        panic(panicValue)
    }
}

...
    var err error
    helpers.DoWithTimeout(func() {
        client, err = sentinel.NewClientCustom("tcp", addr, POOL_SIZE, dial, "smth")
    }, 3*time.Second, "Redis Sentinel communication timeout")
    if err != nil {
        panic(err)
    }