Implement reusing redis client

[jerbob92]

This PR allows you to re-use an existing Redis client with Asynq, this gives you the benifit of less Redis connections and more control over the Redis configuration options since Asynq doesn't provide all the options.

[kamikazechaser]

This is already possible without any changes to asynq: https://pkg.go.dev/github.com/hibiken/asynq?utm_source=godoc#RedisConnOpt

E.g.

package pool

type (
	SharedRedisPoolOpts struct {
		DSN          string
		MinIdleConns int
	}

	SharedRedisPool struct {
		Client *redis.Client
	}
)

func NewSharedRedisPool(o SharedRedisPoolOpts) (*SharedRedisPool, error) {
	redisOpts, err := redis.ParseURL(o.DSN)
	if err != nil {
		return nil, err
	}

	redisOpts.MinIdleConns = o.MinIdleConns
	redisClient := redis.NewClient(redisOpts)

	return &SharedRedisPool{
		Client: redisClient,
	}, nil
}

// Fulfills asynq's Interface
func (r *SharedRedisPool) MakeRedisClient() interface{} {
	return r.Client
}

// Somewhere else when creating an asynq server asynq.NewServer()
// use *pool.SharedRedisPool

[jerbob92]

Thanks! I knew it is already possible but it's not really that clear to users how they can do it and it doesn't protect you against accidental closing of the client.

[kamikazechaser]

how they can do it

Right, it is not documented in the wiki. Maybe it should be added there.

doesn't protect you against accidental closing of the client.

True.

Btw looks like some tests are failing.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (6a7bf2c) 68.37% compared to head (9e548fc) 68.49%.

Files	Patch %	Lines
inspector.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
+ Coverage   68.37%   68.49%   +0.11%     
==========================================
  Files          27       27              
  Lines        3836     3856      +20     
==========================================
+ Hits         2623     2641      +18     
- Misses        929      930       +1     
- Partials      284      285       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jerbob92]

Btw looks like some tests are failing.

Yep, fixed!

[kamikazechaser]

Some feedback on godocs and the Scheduler shutdown behavior with this change.

[kamikazechaser]

// NewInspectorFromRedisClient returns a new instance of Inspector given a redis.UniversalClient
// Warning: The underlying redis connection pool will not be closed by Asynq, you are responsible for closing it.

[kamikazechaser]

// NewClientFromRedisClient returns a new instance of Client given a redis.UniversalClient
// Warning: The underlying redis connection pool will not be closed by Asynq, you are responsible for closing it.

[kamikazechaser]

// NewSchedulerFromRedisClient returns a new instance of Scheduler given a redis.UniversalClient
// The parameter opts is optional, defaults will be used if opts is set to nil.
// Warning: The underlying redis connection pool will not be closed by Asynq, you are responsible for closing it.

[kamikazechaser]

In the case of scheduler.Shutdown, the client is closed by Asynq. If it uses a shared pool, it will return an error (which is unhandled here). Perhaps it should be moved to the following if clause?

If the scheduler ends up using a shared redis connection pool, perhaps it should only be stopped. Right now there doesn't seem to be an option to simply Stop/Pause the scheduler though

[kamikazechaser]

// NewServerFromRedisClient returns a new instance of Server given a redis.UniversalClient
// and server configuration
// Warning: The underlying redis connection pool will not be closed by Asynq, you are responsible for closing it.

[kamikazechaser]

@jerbob92 This is ready to be merged. Could you update the inline docs (as per the suggestions) so that we have a uniform outlook. Thanks.