-
Notifications
You must be signed in to change notification settings - Fork 4
Add Valkey support with parametrized tests #369
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
kibertoad
wants to merge
21
commits into
main
Choose a base branch
from
migrate-to-valkey-glide
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
f9198ff
Add Valkey support and parametrize tests for Redis and Valkey
kibertoad 80ef53b
Fix linting issues in parametrized tests
kibertoad 41f8fec
Add valkey-glide support with RedisClientAdapter
kibertoad 902fc20
Complete valkey-glide integration with full adapter pattern
kibertoad 37915c4
Add full valkey-glide pub/sub support to notification consumers
kibertoad 5efea7e
Update tests to use real GlideClient for Valkey tests
kibertoad 251614e
Implement full Lua script support via invokeScript adapter method
kibertoad 085e334
Update RedisGroupCache tests to use GlideClient
kibertoad 29ea936
feat: fix boolean conversion and add async pub/sub factory pattern
kibertoad cd72c22
feat: Implement valkey-glide pub/sub with multi-callback support
kibertoad 9680847
fix: Resolve valkey-glide pub/sub message routing and error handling
kibertoad 7e24d82
chore: Fix lint warnings and add core dumps to gitignore
kibertoad d6e961d
docs: Add comprehensive valkey-glide documentation
kibertoad 2321c9c
fix: Add valkey-glide as optional peer dependency and fix TypeScript …
kibertoad 2ea2436
fix: Fix TypeScript GlideString type error and update documentation s…
kibertoad ffed180
feat: Use native incr command with valkey-glide instead of Lua scripts
kibertoad aa439c6
feat: Use valkey-glide Batch API for atomic transactions instead of L…
kibertoad c76651e
fix: Change mset signature to accept flat string[] instead of Record
kibertoad c5fba25
docs: Add multi() API design rationale
kibertoad 7d18390
fix: correct mset implementation and setMany execution
kibertoad 33749f1
cleanup
kibertoad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,3 +105,4 @@ dist | |
| .idea | ||
| package-lock.json | ||
| dist | ||
| core | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| # Multi/Batch API Design Decision | ||
|
|
||
| ## Current Implementation | ||
|
|
||
| The current `multi()` signature accepts an array of commands and executes them atomically: | ||
|
|
||
| ```typescript | ||
| interface RedisClientInterface { | ||
| multi?(commands: any[][]): Promise<any> | ||
| } | ||
| ``` | ||
|
|
||
| ### IoRedisClientAdapter | ||
| ```typescript | ||
| async multi(commands: any[][]): Promise<any> { | ||
| return this.client.multi(commands).exec() | ||
| } | ||
| ``` | ||
|
|
||
| ### ValkeyGlideClientAdapter | ||
| ```typescript | ||
| async multi(commands: any[][]): Promise<any> { | ||
| const batch = new Batch(true) // atomic | ||
| // ...add commands to batch... | ||
| return this.client.exec(batch, true) | ||
| } | ||
| ``` | ||
|
|
||
| ## Potential Alternative: Fluent/Chainable API | ||
|
|
||
| A fluent API would look like: | ||
|
|
||
| ```typescript | ||
| interface RedisClientInterface { | ||
| multi?(): MultiPipeline | ||
| } | ||
|
|
||
| interface MultiPipeline { | ||
| incr(key: string): this | ||
| pexpire(key: string, ms: number): this | ||
| set(key: string, value: string, mode?: string, ttl?: number): this | ||
| exec(): Promise<any[]> | ||
| } | ||
| ``` | ||
|
|
||
| ### Pros of Fluent API | ||
| - Matches ioredis native API more closely | ||
| - Allows incremental command building | ||
| - More flexible for complex scenarios | ||
|
|
||
| ### Cons of Fluent API | ||
| - **Doesn't match valkey-glide's Batch API design** | ||
| - Batch is built declaratively, not fluently | ||
| - Would require creating a wrapper class for valkey-glide | ||
| - **Not needed for current use cases** | ||
| - All our usage builds command arrays first | ||
| - No need for incremental chaining in practice | ||
| - **More complex implementation** | ||
| - Need to maintain wrapper class state | ||
| - Need to handle differences between ioredis pipeline and Batch | ||
|
|
||
| ## Current Usage Pattern | ||
|
|
||
| All current usage follows this pattern: | ||
|
|
||
| ```typescript | ||
| // Build command array | ||
| const commands = [ | ||
| ['incr', key], | ||
| ['pexpire', key, ttl], | ||
| ] | ||
|
|
||
| // Execute atomically | ||
| await this.redis.multi(commands) | ||
| ``` | ||
|
|
||
| This pattern: | ||
| - ✅ Works identically for both clients | ||
| - ✅ Clear and explicit | ||
| - ✅ Easy to test | ||
| - ✅ No hidden state in wrapper objects | ||
|
|
||
| ## Recommendation | ||
|
|
||
| **Keep the current array-based API** because: | ||
|
|
||
| 1. **It works perfectly for our use cases** - We always build full command lists upfront | ||
| 2. **It's simpler** - No need for wrapper classes or state management | ||
| 3. **It's portable** - Works the same way for both ioredis and valkey-glide | ||
| 4. **It's testable** - Easy to verify what commands will be executed | ||
| 5. **It's explicit** - Caller sees all commands at once | ||
|
|
||
| ## If Fluent API is Needed in Future | ||
|
|
||
| If we later need a fluent API, we can: | ||
|
|
||
| 1. Keep `multi(commands[][])` for the declarative approach | ||
| 2. Add `createPipeline()` or `createTransaction()` for fluent approach | ||
| 3. Return wrapper class that implements MultiPipeline interface | ||
|
|
||
| This would allow both styles: | ||
|
|
||
| ```typescript | ||
| // Declarative (current) | ||
| await redis.multi([['incr', key], ['pexpire', key, ttl]]) | ||
|
|
||
| // Fluent (future if needed) | ||
| await redis.createTransaction() | ||
| .incr(key) | ||
| .pexpire(key, ttl) | ||
| .exec() | ||
| ``` | ||
|
|
||
| ## Conclusion | ||
|
|
||
| The current implementation is **correct and appropriate** for our needs. | ||
|
|
||
| The array-based API: | ||
| - ✅ Matches our usage patterns | ||
| - ✅ Works consistently across both clients | ||
| - ✅ Is simple and maintainable | ||
| - ✅ Is easy to test and reason about | ||
|
|
||
| **No changes needed** to the multi() API at this time. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.