Skip to content

Fix various commands to work with Redis Cluster #338

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

Merged
merged 21 commits into from
May 28, 2025
Merged

Conversation

abrookins
Copy link
Collaborator

@abrookins abrookins commented May 21, 2025

Description

This PR introduces Redis Cluster support throughout the library by updating internal client abstractions and connection logic. It ensures compatibility with both standalone and clustered Redis deployments.

Key Changes

  • Redis client compatibility

    • Upgraded redis from 5.2.16.1.0, keeping backwards-compatible support for 5.x series
    • Replaced explicit redis.Redis/redis.asyncio.Redis types with union types (SyncRedisClient, AsyncRedisClient, etc.) in redisvl.types
    • Updated all cache, index, and router components to support both standalone and cluster Redis clients (sync and async)
  • Indexing improvements

    • Cluster-safe implementations for FT.CREATE, FT.SEARCH, and FT.DROPINDEX (supposed to be supported, but redis-py 6.1 still isn't sending these to the default node...)
    • Introduced cluster_create_index and cluster_search utility functions to help fill these client gaps
  • Connection handling

    • Refactored RedisConnectionFactory to detect and create cluster clients automatically
    • Improved sync-to-async conversion with type checks -- converting a sync Cluster client to async isn't possible because the connection pool works differently
  • Test infrastructure

    • Added Docker Compose-based Redis Cluster setup for integration tests (tests/cluster-compose.yml)
    • New fixtures: redis_cluster_url, cluster_client
    • New tests validating cluster behavior (test_redis_cluster_support.py)
    • Updated tests that were creating new vectorizers unnecessarily to use the existing session-scoped fixtures (cut down API traffic to HF)
  • Other updates

    • Added tests/data to .gitignore
    • Removed types-redis because its outdated type annotations cause erroneous lint errors
    • Improved TTL and async client cleanup logic
    • Cleaned up types, error handling, and inline documentation

Gotchas

  • I could not get Redis Cluster to come up in CI, only locally, and couldn't determine the problem from stdout/stderr, logs, etc.
  • To unblock other work, I've flagged the cluster tests so they run separately, with --run-cluster-tests.
  • This means Redis Cluster tests are currently turned off in CI

Why

Supporting Redis Cluster enables RedisVL to operate in high-availability, horizontally-scalable environments, making it production-ready for distributed Redis deployments.

This also unblocks us from allowing downstream libraries, such as langgraph-redis, to support Redis Cluster.

Breaking Changes

  • Dropped: types-redis (not compatible with updated redis package)

@abrookins abrookins requested a review from Copilot May 21, 2025 01:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates RedisVL to support Redis Cluster deployments by adding cluster‐aware connection logic, refining client type hints and conversions, and updating tests and Docker Compose configurations for integration testing.

  • Updated client connection factory functions and union types to support both sync and async Redis Cluster clients.
  • Enhanced index, aggregation, cache, and router modules to correctly route commands in cluster mode.
  • Modified tests and Docker configurations to validate and support Redis Cluster behavior.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/integration/* Updated tests to use new client parameters and validate cluster support.
tests/conftest.py & tests/cluster-compose.yml Introduced fixtures and a Docker Compose file for Redis Cluster setup.
redisvl/types.py Added union types for cluster and async clients.
redisvl/redis/connection.py Refactored connection logic to automatically detect cluster URLs and create appropriate clients.
redisvl/redis/utils.py Introduced cluster_create_index and cluster_search functions for cluster compatibility.
redisvl/index/* Modified storage and search index implementations to support cluster commands.
redisvl/extensions/* Adjusted cache and router modules for consistent Redis Cluster client usage.
pyproject.toml Upgraded the redis package dependency and added new type dependency.

- sh
- -c
- |
echo "Waiting for Redis nodes to be available..."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a little much, but it was the result of many iterations on cluster startup failures. I may simplify!

)
yield
except subprocess.CalledProcessError as e:
# Attempt to get logs if setup failed
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, multiple iterations on repeated startup failure 😬

@pytest.fixture(scope="session")
def redis_cluster_url(redis_cluster_container):
# Hard-coded due to Docker issues
return "redis://localhost:7001"
Copy link
Collaborator Author

@abrookins abrookins May 21, 2025

Choose a reason for hiding this comment

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

Currently hard-coded due to issues obtaining the IP at runtime.

Copy link
Collaborator

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

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

Huge effort!!

@abrookins abrookins merged commit 01fd8b3 into main May 28, 2025
104 of 105 checks passed
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.

2 participants