Add NewFromConnection method for Redis #1680
Conversation
Introduced NewFromConnection to create Storage using an existing Redis client, enhancing flexibility. Added benchmarks to test Redis operations with this method.
Updated the Redis README to include the NewFromConnection function, which allows creating a Storage object from an existing Redis client connection.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughA new constructor function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RedisUniversalClient
participant Storage
Client->>RedisUniversalClient: Create or obtain existing client
Client->>Storage: Call NewFromConnection(RedisUniversalClient)
Storage-->>Client: Returns Storage instance using provided client
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
|
I would recommend that if we accept this P.R. for redis we implement it as a standard for other adapters. |
I agree, it would be nice to have this work with any Since most If we do decide to implement it as a standard, I think it would be good enough to allow passing in any struct that implements |
|
This is related to #967 questions is, what do we do with all the other config options on each Storage driver? But yes, this needs to be added to all the drivers. |
|
I appreciate you taking a look at my proposed changes! While I understand there may be discussion around standardizing this approach across all databases, I believe accepting this pull request now would still provide clear benefits: Flexibility for Redis Users A Potential Model for Other Drivers Backwards-Compatible and Low-Risk Looking forward to any final thoughts or suggestions on next steps :) |
|
This feature is essential, since most applications already use Redis connections for various purposes, eliminating the need to create a new one for fiber storage. |
redis/README.md
Outdated
| @@ -21,6 +21,7 @@ A Redis storage driver using [go-redis/redis](https://github.com/go-redis/redis) | |||
| ### Signatures | |||
| ```go | |||
| func New(config ...Config) Storage | 10000|||
| func NewFromConnection(conn redis.UniversalClient) Storage | |||
There was a problem hiding this comment.
Pls add a section with example and short description with the purpose of this new method
There was a problem hiding this comment.
I added an example. Does it look good?
This commit adds a section to the README showing how to create a Storage instance from an existing Redis client, facilitating client connection reuse.
Implemented tests to verify Redis key management functions including set, get, and delete operations. Ensuring data integrity and operation correctness in Redis storage.
redis/README.md
Outdated
| @@ -21,6 +21,7 @@ A Redis storage driver using [go-redis/redis](https://github.com/go-redis/redis) | |||
| ### Signatures | |||
| ```go | |||
| func New(config ...Config) Storage | |||
| func NewFromConnection(conn redis.UniversalClient) Storage | |||
There was a problem hiding this comment.
I added an example. Does it look good?
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new method, NewFromConnection, to create a Redis Storage instance from an existing Redis connection, enhancing flexibility. It also updates benchmark tests and documentation accordingly.
- Added benchmarks for Set, Delete, and Get operations using an existing connection.
- Introduced NewFromConnection in the redis package.
- Updated the README to document the new functionality.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| redis/redis_test.go | Added benchmark and functional tests for the new connection-based creation method. |
| redis/redis.go | Implemented NewFromConnection to wrap an existing Redis client into a Storage instance. |
| redis/README.md | Updated documentation to include usage of NewFromConnection, with a noted signature mismatch. |
Co-authored-b 8000 y: Copilot <175728472+Copilot@users.noreply.github.com>
Introduced NewFromConnection to create Storage using an existing Redis client, enhancing flexibility. Added benchmarks to test Redis operations with this method and updated README.
Summary by CodeRabbit
Documentation
New Features
Tests