Skip to content

feat: added support for redis v5 #1710

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

instanacd
Copy link
Contributor

@instanacd instanacd commented Apr 30, 2025

Redis v5: Changes in this PR

  1. Connection Termination
    In Redis v5, the method client.quit() (or client.QUIT) has been deprecated and replaced by client.close().

  2. Scan Iterator Behavior

    • v4: Iterators yielded individual elements.
    • v5: Iterators now yield entire collections.
  3. Cluster Command Signature Change
    In Redis v5, the method cluster.multi().addCommand() has an updated signature.

    • The second argument is now a boolean (isReadonly), and the actual command array is passed as the third argument.
  4. Connection Pool
    Redis v5 introduces a dedicated RedisClientPool class for managing connection pools.

    • v4: Pooling was handled internally by the RedisClient using an "Isolation Pool".
    • v5: Pooling responsibilities are now abstracted into a separate class.
  5. Internal File Structure Changes
    Redis v5 introduces a new internal structure for the package, requiring updates to how we reference and mock internal modules.

  6. Package Versioning Alignment
    The @redis/client package versioning now aligns with the Redis server version:

    • v4: @redis/client was versioned as 1.x.
    • v5: @redis/client is now versioned as 5.x.

Additional Update

  • The Redis Docker image has been updated to the latest version.

Tasks

References

Release notes: https://github.com/redis/node-redis/releases/tag/redis%405.0.0
Migration guide: https://github.com/redis/node-redis/blob/master/docs/v4-to-v5.md

Ref: https://jsw.ibm.com/browse/INSTA-35744

@instanacd instanacd requested a review from a team as a code owner April 30, 2025 17:32
@aryamohanan
Copy link
Contributor

Let's wait for the currencyJira card to be created.

@aryamohanan aryamohanan self-assigned this May 2, 2025
@aryamohanan
Copy link
Contributor

In v5, Redis moved “Isolation Pool” into RedisClientPool with a createClientPool export—but importing createClientPool throws “Not Found,” so the docs don’t match the package. Opened an issue in redis: redis/node-redis#2936

@aryamohanan aryamohanan marked this pull request as draft May 5, 2025 09:00
@aryamohanan aryamohanan added the WIP label May 5, 2025
@aryamohanan aryamohanan changed the title [Currency Bot] Bumped redis from 4.7.0 to 5.0.0 feat: added support for redis v5 May 5, 2025
@aryamohanan aryamohanan force-pushed the currency-bot-major-redis branch 3 times, most recently from e28b5da to 607a935 Compare May 6, 2025 11:34
@aryamohanan
Copy link
Contributor

The new sentinel API is not working for me created an issue: redis/node-redis#2962

@@ -1,7 +1,7 @@
version: '2'
services:
redis:
image: redis:5.0.14
image: redis:7.4.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Redis Docker image has been updated to the latest version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUA

@@ -111,6 +111,7 @@
"@opentelemetry/instrumentation": "0.50.0",
"@opentelemetry/sdk-node": "^0.49.1",
"@opentelemetry/sdk-trace-base": "^1.22.0",
"@redis/client-v4": "npm:@redis/[email protected]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @redis/client package was introduced with Redis v4 (@redis/client v1). In Redis v5, the versioning of @redis/client was synchronized with Redis, meaning @redis/client v5 corresponds to Redis v5.

To improve the testing setup, I’ve specified @redis/client v4 (corresponding to Redis v4) instead of v1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"@redis/client-v4": "npm:@redis/[email protected]",

v4 = 1.6.0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v4 = 1.6.0?

In v4, the redis/client was at version 1.6.x (see here). With v5, the client version was updated to v5.0 to align its versioning with Redis itself. For simplicity in testing, it is labeled as v4, and I have added a comment in the test file.

Additionally, for redis/client, after version 1.6, the major version jumped directly to v5, with no official stable releases in between—only prerelease versions were published. See: https://www.npmjs.com/package/@redis/client?activeTab=versions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to add

@redis/client: 5.x
@redis/client-4: v1

?

So we test against all legacy package versions and the new releases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea. Make sense!. @redis/client is a direct dependency of the redis package, so it gets installed automatically along with it. However, for better readability and maintainability, I can explicitly include it as well.

@redis/client: v5.x
@redis/client-4: v1.x

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need it for currency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

"redis-v3": "npm:redis@^3.1.2",
"redis-v4": "npm:redis@^4.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I maintain tests for three versions of Redis, and each of these tests involves changes.

@aryamohanan aryamohanan force-pushed the currency-bot-major-redis branch from 88b438a to 10de7ff Compare May 14, 2025 13:53
@aryamohanan
Copy link
Contributor

aryamohanan commented May 14, 2025

As discussed the sentinel support will be added in a separate PR

@aryamohanan aryamohanan marked this pull request as ready for review May 14, 2025 13:56
@aryamohanan aryamohanan removed the WIP label May 15, 2025
@@ -721,8 +771,12 @@ const globalAgent = require('../../../globalAgent');
span => expect(span.n).to.equal('node.http.server'),
span => expect(span.data.http.method).to.equal('GET')
]);
// NOTE: v5 SCAN iterators yield batches of keys, enabling multi-key commands like MGET.
// See: https://github.com/redis/node-redis/blob/master/docs/v4-to-v5.md#scan-iterators
const expectedSpanCount = redisVersion === 'latest' ? 1 : 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QS: Could you please add an assertion for the batch property span.b

Copy link
Contributor

@aryamohanan aryamohanan May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of scanIterators. It's not a batched span. The comment is confusing I will update

await controls.clearIpcMessages();
});

it('should trace blocking commands', () => testBlockingCommand(controls, setupType));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this test. Blocking commands?

               method: 'POST',
                path: '/clearkeys'

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💁‍♀️

@aryamohanan aryamohanan force-pushed the currency-bot-major-redis branch from 10de7ff to e221401 Compare May 16, 2025 09:03
@aryamohanan aryamohanan force-pushed the currency-bot-major-redis branch from c0363e0 to faf4476 Compare May 20, 2025 11:36
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.

3 participants