Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using multiple connection strings #74

Closed
TheGame2500 opened this issue Jul 12, 2018 · 20 comments
Closed

Using multiple connection strings #74

TheGame2500 opened this issue Jul 12, 2018 · 20 comments

Comments

@TheGame2500
Copy link

I have to tcp gateways to my redis db's. How could I use multiple connection strings so that if one fails it failovers to second one?

@SGrondin
Copy link
Owner

I'm not sure, it's definitely a question for the node_redis package, as Bottleneck is simply passing your options through to that package.

@TheGame2500
Copy link
Author

@SGrondin lots of people are asking for this, but it seems like this lib is no longer maintained.
redis/node-redis#574 there are lots of offshoots out there solving the problem though. Is there anyway I can use them without rewriting code in Bottleneck?

@SGrondin
Copy link
Owner

I've started looking into abstracting away the Redis library used by Bottleneck, to make it work with both node_redis and ioredis. At the moment it's not possible to use Bottleneck Clustering without node_redis, but this is my priority.

@TheGame2500
Copy link
Author

@SGrondin not sure if it has to do with work load or your LUA scripts perhaps (surpasses my redis competence at this point), but I get a lot of UnhandledPromiseRejectionWarning: Error: Redis connection to [censored] failed - connect ETIMEDOUT which seem to go away after restarting my application.

I initially thought this is related to a conflict of limiter ids not being unique over my staging/prod environments (which share the same redis db), but even after fixing them I'm getting these errors.

I have another app with even bigger load which used to share the same db, but would not get these errors. According to my compose.io metrics (my redis provider) it looks like the app is simply having too many connections open, but even as I scaled my TCP portals up (gateways to redis), bottleneck simply seems to keep opening connections and never closing them

@TheGame2500
Copy link
Author

Also, @SGrondin have you got any timeline for this?

@SGrondin
Copy link
Owner

I've just started looking into it, I'll need to reproduce it before I can give a timeline. Could you answer the following questions? It will greatly help me reproduce the issue you're seeing.

  • What is your Bottleneck config?
  • Are you using Groups?
  • Are you using JobOptions, if so, what are they?
  • Can you provide a rough estimate to the number of requests/minute are going to the Redis Cluster?
  • When you run the info command on the Redis server, under # Keyspace, how many keys are there?
  • How much memory is allocated to the Redis server?

Thanks!

@edwardupton
Copy link

edwardupton commented Jul 26, 2018

Hi @SGrondin I'm a colleague of @TheGame2500. I thought i'd answer while he is on holiday as our current Bottleneck setup is unstable. Our Redis server (with Compose.com) has a hard limit of 4000 maxclients (concurrent connections), and somehow we are creating 4 new connections a minute - meaning we have to restart the server every 18 hours to disconnect all and start again.

The server is configured to disconnect clients after 10 seconds of idle EXCEPT for pub/sub connections. So the issue could be that we're creating more pub/sub connections.

We are creating a Group like this

const queues = new Bottleneck.Group({
			maxConcurrent: 10,
			minTime: 1100,

			// clustering
			datastore: 'redis',
			clearDatastore: false,
			clientOptions: process.env.REDIS_URL,
		})

There are also a couple of single queues (not using Groups) like

const queue = new Bottleneck({
			maxConcurrent: 5,
			minTime: 1000,

			// clustering
			id: `sheets_${process.env.ENV}`,
			datastore: 'redis',
			clearDatastore: false,
			clientOptions: process.env.REDIS_URL,
		});

We then enqueue requests like this

enqueueRequest : function(requestType, queues, priority, key, ...args) {
		const helpers = require('./helpers/helpers');
		
		const uniqueKey = `${process.env.ENV}-${key}`

		return queues.key(uniqueKey).ready().then(() =>{
			return queues.key(uniqueKey).schedule.apply(queues.key(key), [{priority}, helpers[requestType]].concat(args)); 
		})
	}

It's not clear from your documentation where we should call .disconnect() after each request, or whether the Redis client is kept open for future requests?

~400 requests/minute to Redis
~50 keys at one time (the length of the queue)
256MB Redis memory

@SGrondin
Copy link
Owner

SGrondin commented Jul 26, 2018

Thank you for the very detailed report, it's very helpful. This is my top priority at the moment. It's hard to give a timeframe since I also have a full time job and many other responsibilities, but I'll commit to having a fix within 14 days.

Thank you for trusting me and Bottleneck, reliability has always been the most important "feature" of this project. I'll keep you updated.

@edwardupton
Copy link

@SGrondin Totally understand you have a day job too!

As an update, I set the timeout for the Bottleneck group at 30 seconds, and added an expiration time for each job at 60 seconds. That seems to have fixed the problem of exceeding maximum connections for now.

It seems that, since the Bottleneck settings are stored in Redis, we needed to call updateSettingsto change the timeout, rather than just passing it into the settings object when the queue starts?

And the original request to use ioredis library would still be great.

@SGrondin
Copy link
Owner

SGrondin commented Aug 4, 2018

As an update, I set the timeout for the Bottleneck group at 30 seconds, and added an expiration time for each job at 60 seconds. That seems to have fixed the problem of exceeding maximum connections for now.

Yes, that will help a lot, since it will free up connections as soon as a group key times out. I forgot to document this important point in the Clustering docs. 😞 Note: job expirations don't affect connections.

The problem is that the Redis network protocol is not full duplex, which means only one request can be executed at a time, per connection, even if they don't try to access the same keys. In other words, the requests can't overlap in time: it is strictly Request-Response.

But Redis is single-threaded, so it shouldn't matter, right? Technically yes, but that ignores network latency and Redis Cluster/Sentinel. By creating multiple connections we can increase performance several times over by negating the effects of network latency.

When I first added Clustering to Bottleneck, I decided to favor performance instead of reusing connections, since I expected users to set their own Group timeouts. I thought it would be more common for users to have few a limiters accessed often than a lot of limiters accessed rarely. I was wrong! At the moment, limiters manage their own connections: one for requests and one for pubsub. Obviously, it causes issues when Groups become very large, which is why choosing a good Group timeout value is so important.

I've just finished implementing connection reuse within Groups. Every limiter within a Group will now share the same 2 connections. Standalone limiters will continue to have their own. This setting will be enabled by default and I'll document the tradeoffs. It will be released in the next few days with v2.7.0. I'll update you once it's released.

This work also paves the way to add support for IoRedis. That's the next feature on the roadmap.

It seems that, since the Bottleneck settings are stored in Redis, we needed to call updateSettingsto change the timeout, rather than just passing it into the settings object when the queue starts?

v2.7.0 will let you do:

const queues = new Bottleneck.Group({
			maxConcurrent: 10,
			minTime: 1100,

			// clustering
			datastore: 'redis',
			clearDatastore: false,
			clientOptions: process.env.REDIS_URL,
			timeout: 60 * 1000
		}
)

@edwardupton
Copy link

edwardupton commented Aug 6, 2018 via email

@SGrondin
Copy link
Owner

v2.7.0 has been released, you'll be able to pass the timeout option to a new Group directly.

It also changes how Redis connections are managed. All the limiters created by a Group share the same connection. Standalone limiters have their own connections. Make sure to listen to the error events from Groups, since this is where Redis connectivity errors are sent.

Thanks to the connection changes, you're not forced to set a low timeout value anymore.

I'll update this issue once ioredis is supported.

@TheGame2500
Copy link
Author

@SGrondin thanks! Will update today and let you now how this goes for us!

@TheGame2500
Copy link
Author

TheGame2500 commented Aug 13, 2018

@SGrondin unfortunately went poorly. Lots of ReplyErrors with the folowing message: NOSCRIPT No matching script. Please use EVAL.. Probably script loading / SHA storage is somehow failing, the same wall I've hit when trying to migrate the library to use https://github.com/gosquared/redis-clustr (although at the moment it seemed like an incompatibility between redis-clustr and my redis deployment, so this might send you off track). Reverted to 2.6.0 for now.

LE: seems like the NOSCRIPT command is preceded by an insightful warning: The EVALSHA command contains a "undefined" argument. so I suppose this is the reason there's no loaded script.

@SGrondin
Copy link
Owner

SGrondin commented Aug 13, 2018

I'm investigating this immediately. Can you paste that warning? I'll be able to figure out which call is trying to pass blank arguments.

I've figured out the issue, will release a hotfix today.

@SGrondin
Copy link
Owner

v2.7.1 should fix the NOSCRIPT error. Thank you for letting me know quickly and I'm sorry for not catching this problem. I'm adding tests to ensure it doesn't happen again.

@TheGame2500
Copy link
Author

@SGrondin thanks for the quick fix. No worries, happens to everyone. I've updated to 2.7.1, all seems fine and I'll check how it's going in 30 mins.

@TheGame2500
Copy link
Author

TheGame2500 commented Aug 13, 2018

@SGrondin works fine for now, will let you know if we encounter further issues. Thank you so much for your work!

@SGrondin
Copy link
Owner

Awesome! 🎉 I'm glad to hear that.

I've created #78 to track progress of ioredis support.

@SGrondin
Copy link
Owner

v2.8.0 has been released, it adds support for ioredis, Redis Cluster and Redis Sentinel

  • To use ioredis instead of Node Redis, pass the following option to Bottleneck: { datastore: "ioredis" }. ioredis supports Redis Cluster and Redis Sentinel
  • To use Redis Cluster, pass the following options to Bottleneck: { datastore: "redis", clusterNodes: [nodes] }. See the ioredis cluster docs and the Bottleneck docs for more information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants