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

Redis updates #1328

Merged
merged 4 commits into from
Jun 5, 2024
Merged

Redis updates #1328

merged 4 commits into from
Jun 5, 2024

Conversation

jaymell
Copy link
Contributor

@jaymell jaymell commented Jun 4, 2024

  • Add redisclusterunpooled cache type

    Adds downstream changes to support unpooled redis cluster.
    Simply put, we don't gain anything by using connection pooling
    for clustered redis. The redis cluster client handles establishing
    connections, handling errors, etc., internally. As long as we don't
    make blocking redis requests, we are fine without connection pooling
    in the redis caching layer.

  • Remove rediscluster cache backend

    Note: Also fixed some of the queue tests, which should match
    based on the queue configuration, not the cache config.

  • Rename redisclusterunpooled to rediscluster

    This is now our only clustered cache type, so simplify
    the name to match our prior clustered cache variant.

  • Remove connection pool for redis cache variant

    Replace connection pooling with unpooled
    redis::aio::ConnectionManager. The ConnectionManager uses a single
    multiplexed connection to handle all requests and also handles
    reconnections directly, making connection pooling unnecessary.

    Note that these changes applying only to redis caching, not queuing.
    This is safe in the redis caching layer because we do not make
    blocking calls there. However, we do make blocking calls currently
    in our redis queue implementation, so we must continue to use
    connection pooling there to avoid blocking concurrent requests.

    Also note that names for our many methods of acquiring a RedisPool
    have been standardized for less ambiguity.

@jaymell jaymell force-pushed the redis-updates branch 2 times, most recently from a8dc3c4 to 5a0e289 Compare June 4, 2024 17:53
Adds downstream changes to support unpooled redis cluster.
Simply put, we don't gain anything by using connection pooling
for clustered redis. The redis cluster client handles establishing
connections, handling errors, etc., internally. As long as we don't
make blocking redis requests, we are fine without connection pooling
in the redis caching layer.
Note: Also fixed some of the queue tests, which should match
based on the _queue_ configuration, not the cache config.
This is now our only clustered cache type, so simplify
the name to match our prior clustered cache variant.
Replace connection pooling with unpooled
`redis::aio::ConnectionManager`. The ConnectionManager uses a single
multiplexed connection to handle all requests and also handles
reconnections directly, making connection pooling unnecessary.

Note that these changes applying *only* to redis caching, not queuing.
This is safe in the redis caching layer because we do not make
blocking calls there. However, we *do* make blocking calls currently
in our redis queue implementation, so we must continue to use
connection pooling there to avoid blocking concurrent requests.

Also note that names for our many methods of acquiring a `RedisPool`
have been standardized for less ambiguity.
@jaymell
Copy link
Contributor Author

jaymell commented Jun 4, 2024

I went ahead and lumped this into one PR because the changeset is not really that large. The first commit, the addition of redisclusterunpooled, is just bringing in downstream changes here. The second and third commits are cleanup to remove the no-longer-needed pooled cluster variant. The fourth commit is the most substantive new addition, but is fairly small. Nonetheless, I'm happy to break this into smaller PRs if so desired.

@jaymell jaymell marked this pull request as ready for review June 4, 2024 21:28
@jaymell jaymell requested a review from a team as a code owner June 4, 2024 21:28
@svix-james svix-james merged commit 5f88c1c into svix:main Jun 5, 2024
6 checks passed
svix-lucho added a commit that referenced this pull request Jul 11, 2024
## What's Changed
* Enable redis `tcp_nodelay` by @jaymell in
#1316
* server: Simplify short-circuiting logic for Option::None by
@svix-jplatte in #1318
* Improve database error classification by @svix-jplatte in
#1317
* server: Remove unwraps in redis queue module by @svix-jplatte in
#1319
* Improve error message by @jaymell in
#1320
* Update OpenAPI spec and regenerate libs by @svix-lucho in
#1321
* Add NullableBool function by @dacohen in
#1322
* bridge: Upgrade omniqueue and related dependencies by @svix-jplatte in
#1326
* Make config type, variant names less confusing by @svix-jplatte in
#1327
* Redis updates by @jaymell in
#1328
* More redis updates by @jaymell in
#1329
* bridge: More config cleanup by @svix-jplatte in
#1330
* Python: update deps and switch to ruff. by @tasn in
#1332
* bridge: Update Cargo.lock by @svix-jplatte in
#1334
* Bump locked dependencies again by @svix-jplatte in
#1336
* server: Update Cargo.lock by @svix-jplatte in
#1337
* build(deps): bump braces from 3.0.2 to 3.0.3 in /javascript by
@dependabot in #1335
* bridge: Add Kafka as an input by @svix-jplatte in
#1333
* rust: Add Svix::background_task by @svix-jplatte in
#1341
* bridge: Reduce deno dependencies by @svix-jplatte in
#1343
* rust: Add `Svix::with_token` to allow changing api token by
@svix-aaron1011 in #1339
* build(deps): bump ws from 7.4.6 to 7.5.10 in /javascript by
@dependabot in #1338
* bridge: Upgrade wiremock dev-dependency by @svix-jplatte in
#1344
* bridge: Error refactoring by @svix-jplatte in
#1342
* bridge: Add kafka receiver output by @svix-jplatte in
#1345
* bridge: Cargo manifest cleanup by @svix-jplatte in
#1346
* bridge: Remove build step from CI workflow by @svix-jplatte in
#1347
* Fix `Recover Failed Webhooks` response by @jaymell in
#1349
* Libs(Go): adjust Go linter by @svix-onelson in
#1351
* Libs(Go): pluralize GO_MODULES in superlinter by @svix-onelson in
#1353
* build(deps): bump golang.org/x/net from 0.17.0 to 0.23.0 by
@dependabot in #1294
* `Recover Failed Webhooks` use configurable `until` by @jaymell in
#1352
* Update comments in KafkaConsumer by @svix-jplatte in
#1354
* Replace ctype_digit for PHP 8.1 deprecation of non-string arguments by
@jterry in #1355
* CI: add something to run php tests by @svix-onelson in
#1356
* js: Apply workaround for incomplete fetch support in Cloudflare Worker
by @svix-jplatte in #1359
* Bump certifi from 2023.07.22 to 2024.7.4 in /python by @svix-jplatte
in #1358
* ci: Update GitHub actions revs for php-ci by @svix-jplatte in
#1360
* build(deps): bump zerovec from 0.10.2 to 0.10.4 in /server by
@dependabot in #1361
* Server: bump zerovec-derive by @svix-onelson in
#1362
* bridge: Add kafka to example configs, config tests by @svix-jplatte in
#1357
* Libs: bump spec by @svix-onelson in
#1366

## New Contributors
* @dacohen made their first contribution in
#1322
* @jterry made their first contribution in
#1355

**Full Changelog**:
v1.24.0...v1.25.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants