-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: Redis connection pooling #433
Conversation
@paveltiunov I could use some extra eyes on this in terms of testing, it is working locally but I haven't exercised it as extensively as I wanted - one thing I want to do is to cause redis connections to ramp up and then watch that they are detected as idle and ramp back down. I haven't gone out of my way to write extra tests for this - but I did convert to jest while I was troubleshooting something, hopefully that is ok, it seems to be in use in other areas of this project. I also haven't updated any docs. |
@jcw- Hey Jc! Looks really great! And thanks for migrating tests to jest! Really appreciate that! |
class RedisCacheDriver { | ||
constructor() { | ||
this.redisClient = createRedisClient(process.env.REDIS_URL); | ||
constructor(pool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible for the constructor options to be an object, with a property pool
instead? This would be helpful in the future, in case we need to add any more parameters to instantiate a Redis driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hassankhan Yeah. Great idea! I think we should provide options object to RedisPool
instead of arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…b-analytics * 'web-analytics' of github.com:statsbotco/cube.js: (94 commits) Update dependencies Update dependencies Update web-analytics v0.18.0 fix: Redis query queue locking redesign Update Subquery.md tests: Cleanup redis pool properly in tests (#458) Debug CI Update client core dependency for vue tests Fix vue tests and client build Fix tests race conditions fix: Handle multiple occurrences in the first event of a funnel: conversion percent discrepancies. feat: Redis connection pooling (#433) Thanks to @jcw! build(deps): bump moment-range from 4.0.1 to 4.0.2 (#442) feat: Add role parameter to driver options (#448) Thanks to @smbkr! build(deps-dev): bump request from 2.88.0 to 2.88.2 (#441) build(deps): bump recharts from 1.7.1 to 1.8.5 (#446) build(deps): bump sqlite3 from 4.1.0 to 4.1.1 (#445) build(deps): bump tslib from 1.9.3 to 1.11.0 (#440) build(deps-dev): bump http-server from 0.11.1 to 0.12.1 (#437) ...
Check List
Issue Reference this PR resolves
Fixes #104
Description of Changes Made (if issue reference is not provided)
Adds a connection pool class based on
generic-pool
. By default, uses (min=2, max=10). If max is set to zero (e.g. through the env var), then the pool is disabled and it falls back to prior behavior (non-pooled).A single instance of the connection pool is created in the
QueryOrchestrator
and is wired up to two final destinations:RedisQueueDriverConnection
RedisCacheDriver