Skip to content

Improve Deckard integration with Redis Clusters#66

Draft
Copilot wants to merge 5 commits intomainfrom
copilot/fix-13
Draft

Improve Deckard integration with Redis Clusters#66
Copilot wants to merge 5 commits intomainfrom
copilot/fix-13

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 5, 2025

This PR implements Redis Cluster support for Deckard while maintaining full backward compatibility with single-node Redis deployments.

Problem

Deckard's Lua scripts access multiple keys that may hash to different slots in a Redis Cluster, causing script execution to fail. Redis Cluster requires all keys accessed by a Lua script to be on the same hash slot for atomic operations.

Solution

Implemented a hash tag strategy that ensures all queue-related keys land on the same Redis Cluster slot:

Before (Single-node only):

  • deckard:queue:myqueue
  • deckard:queue:myqueue:tmp
  • deckard:queue:myqueue:lock_ack

After (Cluster-compatible):

  • deckard:queue:{myqueue}
  • deckard:queue:{myqueue}:tmp
  • deckard:queue:{myqueue}:lock_ack

The {myqueue} hash tag ensures atomic operations across all related keys.

Changes

Configuration Support

  • Added redis.cluster.mode and redis.cluster.addresses configuration options
  • Support for both redis.Client and redis.ClusterClient via unified interface
  • Environment variable support: DECKARD_REDIS_CLUSTER_MODE, DECKARD_REDIS_CLUSTER_ADDRESSES

Key Naming Strategy

  • Modified key generation methods to use hash tags when cluster mode is enabled
  • Maintains backward compatibility - single-node deployments unchanged
  • Applied to all Redis operations: queue pools, locks, and general cache keys

Testing Infrastructure

  • Redis Cluster Docker Compose setup with 6 nodes (3 masters, 3 replicas)
  • Comprehensive integration tests validating Lua script compatibility
  • Unit tests for both single-node and cluster key generation
  • Automated cluster setup script for local development

Documentation

  • Complete Redis Cluster configuration guide
  • Migration instructions from single-node to cluster
  • Docker Compose examples and troubleshooting guide
  • Performance considerations and best practices

Configuration Examples

Single-node (default, unchanged):

cache:
  type: REDIS
redis:
  address: localhost
  port: 6379

Redis Cluster (new):

cache:
  type: REDIS
redis:
  cluster:
    mode: true
    addresses:
      - redis-node-1:6379
      - redis-node-2:6379
      - redis-node-3:6379

Backward Compatibility

  • ✅ Zero breaking changes to existing API
  • ✅ Single-node Redis deployments work unchanged
  • ✅ All existing tests pass
  • ✅ Default configuration remains single-node mode
  • ✅ Existing key naming preserved for single-node mode

Testing

Run cluster integration tests:

./scripts/setup-redis-cluster.sh
export REDIS_CLUSTER_TEST=1
go test -v ./internal/queue/cache/ -run Cluster

Fixes #13.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits June 5, 2025 17:37
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Copilot AI changed the title [WIP] Improve Deckard integration with Redis Clusters Improve Deckard integration with Redis Clusters Jun 5, 2025
Copilot AI requested a review from lucasoares June 5, 2025 17:45
@lucasoares
Copy link
Copy Markdown
Member

@copilot re-run checks so I can check if tests pass

Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 5, 2025

@copilot re-run checks so I can check if tests pass

Checks re-triggered in commit 6245e08. The CI/CD pipeline should now run the full test suite for the Redis Cluster implementation.

Comment thread docker/docker-compose.yml
Comment on lines +77 to +78
redis-cluster-node-1:7000
redis-cluster-node-2:7001
Copy link
Copy Markdown
Collaborator

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 ?


### Redis Cluster
Keys use hash tags to ensure co-location:
- `deckard:queue:{myqueue}`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 ?
call it {clusterhash} for example.

and change this terminology in all the documentations you have made.

Comment on lines +599 to +606
var redisKey string
if cache.clusterMode {
redisKey = fmt.Sprint("deckard:{", key, "}")
} else {
redisKey = fmt.Sprint("deckard:", key)
}

cmd := cache.Client.Get(context.Background(), redisKey)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is repeated twice, maybe transform it into a method ?

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.

Improve Deckard integration with Redis Clusters

3 participants