feat(redis-kv): replace redis with (potentially) more stable ioredis #267
Conversation
hlolli
changed the title
hlolli
force-pushed
the
feature/PE-7414_ioredis
branch
from
a14411f to
b514290
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
error @permaweb/[email protected]: The engine "yarn" is incompatible with this module. Expected version "please-use-npm". Got "1.22.22" 📝 WalkthroughWalkthroughThe pull request involves migrating the Redis client library from Changes
Sequence DiagramsequenceDiagram
participant App
participant RedisKvStore
participant Redis
App->>RedisKvStore: Create instance with redisUrl
RedisKvStore->>Redis: Create new Redis client
Redis-->>RedisKvStore: Client initialized
App->>RedisKvStore: get(key)
RedisKvStore->>Redis: getBuffer(key)
Redis-->>RedisKvStore: Return value
RedisKvStore-->>App: Return value
App->>RedisKvStore: set(key, value, ttl)
RedisKvStore->>Redis: set(key, value, ttl)
Redis-->>RedisKvStore: Confirm set
RedisKvStore-->>App: Confirm set
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🛑 Comments failed to post (3)
src/store/redis-kv-store.ts (3)
19-19:
⚠️ Potential issueCorrect the import statement for ioredis
The
ioredislibrary exportsRedisas the default export. Therefore, the import statement should use the default import syntax.Apply this diff to fix the import statement:
-import { Redis } from 'ioredis'; +import Redis from 'ioredis';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import Redis from 'ioredis';
42-45: 🛠️ Refactor suggestion
Simplify Redis client initialization and ensure TLS is correctly configured
You can simplify the Redis client initialization by passing the
redisUrldirectly to theRedisconstructor. This approach allowsioredisto handle the parsing of the URL and automatically configure the connection options, including enabling TLS if therediss://protocol is used.Apply this diff to simplify the client initialization:
- const redisUrlObject = new URL(redisUrl); - this.client = new Redis({ - host: redisUrlObject.hostname, - port: Number(redisUrlObject.port), - tls: {}, - }); + this.client = new Redis(redisUrl);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.this.client = new Redis(redisUrl);
66-66:
⚠️ Potential issueReplace
getBufferwithgetand enablereturnBuffersoption in client configurationThe method
getBufferdoes not exist inioredis. To retrieve buffer values, setreturnBuffers: truein the client options and usethis.client.get(key).Apply these diffs to fix the issue:
In client initialization:
- this.client = new Redis(redisUrl); + this.client = new Redis(redisUrl, { returnBuffers: true });In the
getmethod:- const value = await this.client.getBuffer(key); + const value = await this.client.get(key);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/store/redis-kv-store.ts (2)
82-82: Add error handling for Redis operations.While the set implementation is correct, consider adding error handling to properly handle Redis operation failures.
- await this.client.set(key, buffer, 'EX', this.ttlSeconds); + try { + await this.client.set(key, buffer, 'EX', this.ttlSeconds); + } catch (error) { + this.log.error('Failed to set key in Redis', { + key, + error: error.message, + }); + metrics.redisErrorCounter.inc(); + throw error; + }
42-46: Add connection timeout configuration.To improve stability (PR objective), consider adding connection timeout settings.
this.client = new Redis({ host: redisUrlObject.hostname, port: Number(redisUrlObject.port), tls: {}, + connectTimeout: 10000, // 10 seconds + maxRetriesPerRequest: 3, + retryStrategy: (times: number) => { + const delay = Math.min(times * 200, 2000); + return delay; + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)src/init/resolvers.ts(0 hunks)src/store/redis-kv-store.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/init/resolvers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (2)
src/store/redis-kv-store.ts (2)
18-19: LGTM! Clean import replacement.The migration from
redistoioredistypes and imports is clean and correctly implemented.Also applies to: 25-25
66-67: LGTM! Proper buffer handling.The change to
getBufferwith null coalescing is a clean implementation that correctly handles binary data and undefined cases.
src/store/redis-kv-store.ts
Outdated
| const redisUrlObject = new URL(redisUrl); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for URL parsing.
The URL parsing could fail with invalid URLs. Consider adding try-catch block to handle potential URL parsing errors.
- const redisUrlObject = new URL(redisUrl);
+ let redisUrlObject: URL;
+ try {
+ redisUrlObject = new URL(redisUrl);
+ } catch (error) {
+ throw new Error(`Invalid Redis URL: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const redisUrlObject = new URL(redisUrl); | |
| let redisUrlObject: URL; | |
| try { | |
| redisUrlObject = new URL(redisUrl); | |
| } catch (error) { | |
| throw new Error(`Invalid Redis URL: ${error.message}`); | |
| } | |
There was a problem hiding this comment.
that's a good idea, appending this
hlolli
force-pushed
the
feature/PE-7414_ioredis
branch
3 times, most recently
from
6f4d413 to
292a612
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #267 +/- ##
========================================
Coverage 71.75% 71.75%
========================================
Files 37 37
Lines 9280 9280
Branches 537 537
========================================
Hits 6659 6659
Misses 2617 2617
Partials 4 4 ☔ View full report in Codecov by Sentry. |
hlolli
force-pushed
the
feature/PE-7414_ioredis
branch
from
292a612 to
04a1bfd
Compare
|
does ioredis support the |
sure enough - it does! https://github.com/redis/ioredis?tab=readme-ov-file#connect-to-redis
|
|
@dtfiedler ok given this nice finding, I may want to revisit if the hostname property actually contains the protocol, but this also means for us that the url alone could replace the tls on/off switch, almost like postgresql, 1 url for user/pw/options and all. |
|
@dtfiedler the power of the url is such designed, that I think we'll be making ours and the users life simpler by passing a single url string into the constructor and not parse the url. I'm adjusting the PR Different issue: I do feel we should be less forgiving if the connection fails for any reason, at least initially, to crash the application if it fails, it's not easy to discover this from the logs alone imo. |
hlolli
force-pushed
the
feature/PE-7414_ioredis
branch
from
04a1bfd to
566a2b1
Compare
hlolli
force-pushed
the
feature/PE-7414_ioredis
branch
from
566a2b1 to
1824fc7
Compare
I'd propose according to https://github.com/redis/ioredis#tls-options tls isn't a natively supported thing in redis but users can set their custom ca file in case of proxies. But in most cases it's not needed. But an option to pass custom ca file could give away for enableTls boolean.