-
Notifications
You must be signed in to change notification settings - Fork 5
Improve Deckard integration with Redis Clusters #66
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
Draft
Copilot
wants to merge
5
commits into
main
Choose a base branch
from
copilot/fix-13
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.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5f1ac58
Initial plan for issue
Copilot 2a2612a
Initial analysis and plan for Redis Cluster integration
Copilot f0602da
Add Redis Cluster support with hash tag key naming
Copilot bdb42e8
Add Redis Cluster documentation and testing scripts
Copilot 6245e08
Trigger CI checks for Redis Cluster implementation
Copilot 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
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,169 @@ | ||
| # Redis Cluster Configuration | ||
|
|
||
| This document explains how to configure Deckard to work with Redis Clusters. | ||
|
|
||
| ## Overview | ||
|
|
||
| Deckard now supports both single-node Redis and Redis Cluster deployments. When Redis Cluster mode is enabled, Deckard uses hash tags in key naming to ensure all queue-related keys land on the same cluster slot, enabling atomic operations via Lua scripts. | ||
|
|
||
| ## Configuration | ||
|
|
||
| ### Single Node Redis (Default) | ||
|
|
||
| This is the traditional configuration that works with a single Redis instance: | ||
|
|
||
| ```yaml | ||
| cache: | ||
| type: REDIS | ||
| redis: | ||
| address: localhost | ||
| port: 6379 | ||
| password: "" # optional | ||
| db: 0 # optional | ||
| ``` | ||
|
|
||
| Environment variables: | ||
| ```bash | ||
| DECKARD_CACHE_TYPE=REDIS | ||
| DECKARD_REDIS_ADDRESS=localhost | ||
| DECKARD_REDIS_PORT=6379 | ||
| DECKARD_REDIS_PASSWORD=mypassword # optional | ||
| DECKARD_REDIS_DB=0 # optional | ||
| ``` | ||
|
|
||
| ### Redis Cluster | ||
|
|
||
| To enable Redis Cluster support: | ||
|
|
||
| ```yaml | ||
| cache: | ||
| type: REDIS | ||
| redis: | ||
| cluster: | ||
| mode: true | ||
| addresses: | ||
| - redis-node-1:6379 | ||
| - redis-node-2:6379 | ||
| - redis-node-3:6379 | ||
| ``` | ||
|
|
||
| Environment variables: | ||
| ```bash | ||
| DECKARD_CACHE_TYPE=REDIS | ||
| DECKARD_REDIS_CLUSTER_MODE=true | ||
| DECKARD_REDIS_CLUSTER_ADDRESSES=redis-node-1:6379,redis-node-2:6379,redis-node-3:6379 | ||
| DECKARD_REDIS_PASSWORD=mypassword # optional, if cluster requires auth | ||
| ``` | ||
|
|
||
| ## Key Naming Differences | ||
|
|
||
| ### Single Node Redis | ||
| Keys are named without hash tags: | ||
| - `deckard:queue:myqueue` | ||
| - `deckard:queue:myqueue:tmp` | ||
| - `deckard:queue:myqueue:lock_ack` | ||
|
|
||
| ### Redis Cluster | ||
| Keys use hash tags to ensure co-location: | ||
| - `deckard:queue:{myqueue}` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since you are using {myqueue} to refer to a hash tag, this templated name should be improved to be more mnemonic dont you think so ? and change this terminology in all the documentations you have made. |
||
| - `deckard:queue:{myqueue}:tmp` | ||
| - `deckard:queue:{myqueue}:lock_ack` | ||
|
|
||
| The `{myqueue}` hash tag ensures all keys for the same queue hash to the same cluster slot. | ||
|
|
||
| ## Migration from Single Node to Cluster | ||
|
|
||
| ### Option 1: Fresh Deployment | ||
| 1. Deploy new Deckard instance with cluster configuration | ||
| 2. Migrate data using your preferred method | ||
| 3. Switch traffic to new deployment | ||
|
|
||
| ### Option 2: In-place Migration (Advanced) | ||
| ⚠️ **Warning**: This approach requires careful planning and testing. | ||
|
|
||
| 1. Ensure your Redis cluster is properly configured | ||
| 2. Update Deckard configuration to enable cluster mode | ||
| 3. Restart Deckard instance | ||
|
|
||
| Note: Existing keys will remain in the old format and will need manual migration if cross-key atomicity is required. | ||
|
|
||
| ## Docker Compose Example | ||
|
|
||
| ```yaml | ||
| version: "3.8" | ||
|
|
||
| services: | ||
| deckard: | ||
| image: blipai/deckard:latest | ||
| environment: | ||
| - DECKARD_CACHE_TYPE=REDIS | ||
| - DECKARD_REDIS_CLUSTER_MODE=true | ||
| - DECKARD_REDIS_CLUSTER_ADDRESSES=redis-node-1:6379,redis-node-2:6379,redis-node-3:6379 | ||
| depends_on: | ||
| - redis-node-1 | ||
| - redis-node-2 | ||
| - redis-node-3 | ||
|
|
||
| redis-node-1: | ||
| image: redis:7.0 | ||
| command: redis-server --port 6379 --cluster-enabled yes --cluster-config-file nodes.conf --cluster-node-timeout 5000 | ||
| ports: | ||
| - "6379:6379" | ||
|
|
||
| redis-node-2: | ||
| image: redis:7.0 | ||
| command: redis-server --port 6379 --cluster-enabled yes --cluster-config-file nodes.conf --cluster-node-timeout 5000 | ||
| ports: | ||
| - "6380:6379" | ||
|
|
||
| redis-node-3: | ||
| image: redis:7.0 | ||
| command: redis-server --port 6379 --cluster-enabled yes --cluster-config-file nodes.conf --cluster-node-timeout 5000 | ||
| ports: | ||
| - "6381:6379" | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| ### Local Development with Docker | ||
| Use the provided script to set up a local Redis cluster: | ||
|
|
||
| ```bash | ||
| ./scripts/setup-redis-cluster.sh | ||
| ``` | ||
|
|
||
| ### Running Cluster Tests | ||
| ```bash | ||
| export REDIS_CLUSTER_TEST=1 | ||
| go test -v ./internal/queue/cache/ -run Cluster | ||
| ``` | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| ### Common Issues | ||
|
|
||
| **1. "redis.cluster.addresses must be specified" error** | ||
| - Ensure `DECKARD_REDIS_CLUSTER_ADDRESSES` is set when cluster mode is enabled | ||
|
|
||
| **2. "CROSSSLOT Keys in request don't hash to the same slot" error** | ||
| - This indicates the hash tag implementation may have an issue | ||
| - Verify all keys for a queue use the same hash tag format | ||
|
|
||
| **3. Connection timeouts to cluster** | ||
| - Ensure all cluster nodes are accessible from Deckard | ||
| - Check network connectivity and firewall rules | ||
| - Verify cluster is properly initialized | ||
|
|
||
| ### Performance Considerations | ||
|
|
||
| - Redis Cluster may have slightly higher latency than single-node due to network hops | ||
| - Hash tags ensure optimal key distribution while maintaining atomicity | ||
| - Monitor cluster node resource usage and scale as needed | ||
|
|
||
| ### Best Practices | ||
|
|
||
| 1. **Always use odd number of master nodes** (3, 5, 7) for proper quorum | ||
| 2. **Set up Redis Cluster with replicas** for high availability | ||
| 3. **Monitor cluster health** using Redis Cluster commands | ||
| 4. **Test failover scenarios** in staging environments | ||
| 5. **Use consistent hashing strategy** across all deployments | ||
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,120 @@ | ||
| # Redis Cluster Integration Plan | ||
|
|
||
| ## Overview | ||
|
|
||
| This document outlines the plan to improve Deckard's integration with Redis Clusters while maintaining backward compatibility with single-node Redis deployments. | ||
|
|
||
| ## Problem Statement | ||
|
|
||
| Deckard currently uses Lua scripts for atomic operations on Redis, but these scripts access multiple keys that may hash to different slots in a Redis Cluster, causing the scripts to fail. Redis Cluster requires all keys accessed by a Lua script to be on the same hash slot. | ||
|
|
||
| ## Current Key Naming Pattern | ||
|
|
||
| For a queue named `myqueue`, Deckard creates these keys: | ||
| - `deckard:queue:myqueue` (active pool) | ||
| - `deckard:queue:myqueue:tmp` (processing pool) | ||
| - `deckard:queue:myqueue:ack` (ack lock pool) | ||
| - `deckard:queue:myqueue:nack` (nack lock pool) | ||
| - `deckard:queue:myqueue:ack:score` (ack score pool) | ||
| - `deckard:queue:myqueue:nack:score` (nack score pool) | ||
|
|
||
| ## Redis Cluster Constraints | ||
|
|
||
| 1. All keys in a Lua script must hash to the same slot | ||
| 2. Keys must use hash tags to control slot assignment | ||
| 3. Scripts can only access keys explicitly provided as KEYS arguments | ||
| 4. No programmatically-generated key names in scripts | ||
|
|
||
| ## Solution: Hash Tags | ||
|
|
||
| Modify key naming to use Redis hash tags, ensuring all queue-related keys hash to the same slot: | ||
|
|
||
| ### New Key Naming Pattern (Cluster-Compatible) | ||
| - `deckard:queue:{myqueue}` (active pool) | ||
| - `deckard:queue:{myqueue}:tmp` (processing pool) | ||
| - `deckard:queue:{myqueue}:ack` (ack lock pool) | ||
| - `deckard:queue:{myqueue}:nack` (nack lock pool) | ||
| - `deckard:queue:{myqueue}:ack:score` (ack score pool) | ||
| - `deckard:queue:{myqueue}:nack:score` (nack score pool) | ||
|
|
||
| The `{myqueue}` hash tag ensures all keys for the same queue land on the same Redis Cluster slot. | ||
|
|
||
| ## Implementation Plan | ||
|
|
||
| ### Phase 1: Configuration Support | ||
| - [ ] Add Redis Cluster configuration option | ||
| - [ ] Add cluster-aware key naming toggle | ||
| - [ ] Support both legacy and cluster-compatible modes | ||
|
|
||
| ### Phase 2: Key Naming Strategy | ||
| - [ ] Modify key generation functions to use hash tags when cluster mode is enabled | ||
| - [ ] Ensure backward compatibility with existing deployments | ||
| - [ ] Update all key generation methods in RedisCache | ||
|
|
||
| ### Phase 3: Integration Testing | ||
| - [ ] Create Redis Cluster docker-compose setup | ||
| - [ ] Add integration tests for cluster functionality | ||
| - [ ] Validate all Lua scripts work with clustered Redis | ||
| - [ ] Test migration scenarios | ||
|
|
||
| ### Phase 4: Documentation & Deployment | ||
| - [ ] Update deployment documentation | ||
| - [ ] Add Redis Cluster configuration examples | ||
| - [ ] Document migration path from single-node to cluster | ||
|
|
||
| ## Backward Compatibility | ||
|
|
||
| The implementation will: | ||
| 1. Default to legacy key naming for existing deployments | ||
| 2. Require explicit configuration to enable cluster-compatible mode | ||
| 3. Provide migration tools/documentation for upgrading existing deployments | ||
| 4. Maintain API compatibility | ||
|
|
||
| ## Testing Strategy | ||
|
|
||
| 1. **Unit Tests**: Validate key naming functions | ||
| 2. **Integration Tests**: Test against both single-node and cluster Redis | ||
| 3. **Migration Tests**: Verify smooth transition scenarios | ||
| 4. **Performance Tests**: Ensure no degradation in performance | ||
|
|
||
| ## Risks & Mitigations | ||
|
|
||
| ### Risk: Breaking existing deployments | ||
| **Mitigation**: Feature flag and backward compatibility mode | ||
|
|
||
| ### Risk: Performance impact from hash tags | ||
| **Mitigation**: Benchmark testing and optional enablement | ||
|
|
||
| ### Risk: Complex migration path | ||
| **Mitigation**: Clear documentation and migration tools | ||
|
|
||
| ## Configuration Examples | ||
|
|
||
| ### Single Node (Legacy - Default) | ||
| ```yaml | ||
| cache: | ||
| type: REDIS | ||
| redis_cluster_mode: false | ||
| redis: | ||
| address: localhost | ||
| port: 6379 | ||
| ``` | ||
|
|
||
| ### Redis Cluster | ||
| ```yaml | ||
| cache: | ||
| type: REDIS | ||
| redis_cluster_mode: true | ||
| redis_cluster_addresses: | ||
| - redis-node-1:6379 | ||
| - redis-node-2:6379 | ||
| - redis-node-3:6379 | ||
| ``` | ||
|
|
||
| ## Success Criteria | ||
|
|
||
| 1. All existing Lua scripts work with Redis Cluster | ||
| 2. No breaking changes to existing single-node deployments | ||
| 3. Comprehensive test coverage for both modes | ||
| 4. Clear documentation for cluster deployment | ||
| 5. Performance parity with single-node deployments |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make it easier to assimilate by starting the node count from 0 so that it conforms to the ports final digits ?