Support redis/valkey cluster mode #1650
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/webapp/app/env.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/services/authorizationRateLimitMiddleware.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/webapp/app/redis.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
WalkthroughThis pull request introduces comprehensive changes to Redis configuration across the application, focusing on adding support for Redis cluster mode. The modifications span multiple files in the webapp, introducing new environment schema properties, a centralized Redis client creation function, and updated configuration options for rate limiting, caching, and pub/sub services. The changes enable more flexible Redis connection handling, with explicit support for TLS and cluster mode settings. Changes
Sequence DiagramsequenceDiagram
participant Env as Environment Config
participant RedisClient as Redis Client Creator
participant Service as Application Service
Env->>RedisClient: Provide connection options
RedisClient->>RedisClient: Determine cluster mode
alt Cluster Mode Enabled
RedisClient->>Service: Create Cluster Client
else Standalone Mode
RedisClient->>Service: Create Standard Redis Client
end
Service->>Service: Use Redis Client
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/webapp/app/redis.server.ts (2)
4-13: Enhance type safety for RedisWithClusterOptions.The type definition could be more robust:
- Port should be restricted to valid numbers
- Host should be required when not using default values
- Password should be required for production environments
Consider enhancing the type definition:
export type RedisWithClusterOptions = { - host?: string; - port?: number; + host: string; + port: number; username?: string; - password?: string; + password: string; tlsDisabled?: boolean; clusterMode?: boolean; clusterOptions?: Omit<ClusterOptions, "redisOptions">; keyPrefix?: string; };
42-42: Make enableAutoPipelining configurable.The
enableAutoPipeliningoption is hardcoded totruefor both standalone and cluster modes. This might not be optimal for all use cases.Make it configurable:
export type RedisWithClusterOptions = { // ... other options + enableAutoPipelining?: boolean; }; // In the cluster client creation - enableAutoPipelining: true, + enableAutoPipelining: options.enableAutoPipelining ?? true, // In the standalone client creation - enableAutoPipelining: true, + enableAutoPipelining: options.enableAutoPipelining ?? true,Also applies to: 59-59
apps/webapp/app/services/apiRateLimit.server.ts (1)
12-13: Standardize boolean environment variable comparisons.The code uses inconsistent styles for boolean environment variables:
=== "true"for TLS=== "1"for cluster modeConsider standardizing to one style across all boolean environment variables:
- tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true", - clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", + tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "1", + clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",apps/webapp/app/env.server.ts (1)
122-122: LGTM!The environment variables for Redis cluster mode are well-defined:
- Consistent naming pattern across different Redis usages (rate limit, cache, pubsub).
- Safe default value of "0" (disabled).
- Proper type definitions using Zod schema.
Consider adding documentation for the cluster mode variables.
Similar to other environment variables in the file, consider adding JSDoc comments to document the purpose and accepted values for the cluster mode variables:
+ /** + * Enable Redis cluster mode for rate limiting. + * @example "0" - disabled (default) + * @example "1" - enabled + */ RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED: z.string().default("0"), + /** + * Enable Redis cluster mode for caching. + * @example "0" - disabled (default) + * @example "1" - enabled + */ CACHE_REDIS_CLUSTER_MODE_ENABLED: z.string().default("0"), + /** + * Enable Redis cluster mode for pub/sub. + * @example "0" - disabled (default) + * @example "1" - enabled + */ PUBSUB_REDIS_CLUSTER_MODE_ENABLED: z.string().default("0"),Also applies to: 152-152, 182-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/webapp/app/env.server.ts(3 hunks)apps/webapp/app/redis.server.ts(1 hunks)apps/webapp/app/services/apiRateLimit.server.ts(1 hunks)apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts(3 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/services/rateLimiter.server.ts(3 hunks)apps/webapp/app/services/realtimeClient.server.ts(2 hunks)apps/webapp/app/services/realtimeClientGlobal.server.ts(1 hunks)apps/webapp/app/services/unkey/redisCacheStore.server.ts(1 hunks)apps/webapp/app/v3/eventRepository.server.ts(6 hunks)apps/webapp/app/v3/marqs/devPubSub.server.ts(1 hunks)apps/webapp/app/v3/services/projectPubSub.server.ts(1 hunks)apps/webapp/app/v3/utils/zodPubSub.server.ts(5 hunks)docker/docker-compose.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (16)
apps/webapp/app/redis.server.ts (1)
43-43: Review TLS configuration security implications.The current TLS configuration is very basic and might not provide adequate security:
- No CA certificates configuration
- No cert/key configuration for client authentication
- No TLS version or cipher suite restrictions
Verify TLS configuration requirements:
Consider enhancing TLS configuration:
- ...(options.tlsDisabled ? {} : { tls: {} }), + ...(options.tlsDisabled ? {} : { + tls: { + // Add when needed: + // ca: options.tlsCa, + // cert: options.tlsCert, + // key: options.tlsKey, + // minVersion: 'TLSv1.2', + } + }),Also applies to: 60-60
apps/webapp/app/services/unkey/redisCacheStore.server.ts (1)
4-4: LGTM! The Redis client initialization changes look good.The changes consistently implement cluster mode support through:
- Updated type imports
- Type-safe client configuration
- Centralized client creation
However, we should verify the Redis connection parameters are properly configured for cluster mode.
Also applies to: 7-7, 14-14, 17-17
✅ Verification successful
Redis cluster mode configuration is properly implemented
The codebase consistently uses configuration objects and dependency injection, making it fully compatible with both standalone and cluster modes. All Redis client initializations follow the centralized configuration pattern, ensuring consistent behavior across different services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Redis cluster configuration consistency across the codebase # Look for any hardcoded Redis configuration that might conflict with cluster mode rg -g '*.{ts,js}' -A 3 'new Redis\(' .Length of output: 4658
apps/webapp/app/services/rateLimiter.server.ts (2)
36-37: Standardize boolean environment variable comparisons.Same inconsistency in boolean environment variable comparison styles as in apiRateLimit.server.ts.
74-77: LGTM! The Redis client creation is well-implemented.The changes correctly:
- Update the function signature to support cluster mode
- Use a meaningful identifier for the Redis client
- Maintain type safety
apps/webapp/app/v3/utils/zodPubSub.server.ts (2)
26-26: LGTM! The Redis client type updates are correct.The changes properly update the Redis client types to support cluster mode while maintaining type safety.
Also applies to: 107-107
39-39: Consider connection pooling for publisher and subscriber clients.Both the publisher and subscriber are creating separate Redis clients with the same configuration. Consider implementing connection pooling to optimize Redis connections, especially in cluster mode.
Also applies to: 116-116
apps/webapp/app/services/authorizationRateLimitMiddleware.server.ts (2)
12-12: LGTM!The import of
RedisWithClusterOptionsfrom the centralized Redis configuration module is appropriate.
171-172: LGTM!The Redis configuration correctly includes cluster mode and TLS settings:
tlsDisabledis properly derived from the environment variable.clusterModeis properly derived from the environment variable.apps/webapp/app/services/realtimeClient.server.ts (2)
6-6: LGTM!The changes correctly update the imports and type definitions to support Redis cluster mode:
- Imports the necessary types and functions from the centralized Redis module.
- Updates
RealtimeClientOptionsto use the new Redis configuration type.Also applies to: 14-14
30-30: LGTM!The Redis client initialization is properly updated:
- Type is correctly updated to use
RedisClient.- Client is initialized using
createRedisClientwith appropriate namespace.Also applies to: 35-35
apps/webapp/app/services/platform.v3.server.ts (1)
46-47: LGTM!The Redis configuration correctly includes cluster mode and TLS settings:
tlsDisabledis properly derived from the environment variable.clusterModeis properly derived from the environment variable.apps/webapp/app/v3/eventRepository.server.ts (4)
34-34: LGTM!The import of Redis-related types and the centralized client creation function aligns with the PR objective of supporting Redis cluster mode.
100-100: LGTM!The update to use
RedisWithClusterOptionstype enables Redis cluster mode configuration.
203-203: LGTM!The Redis client initialization is properly refactored to use the centralized client creation function with a descriptive name.
Also applies to: 221-221
992-992: LGTM!The Redis subscriber initialization and configuration changes properly support TLS and cluster mode settings.
Also applies to: 1150-1151
docker/docker-compose.yml (1)
8-13: LGTM!The Redis cluster volume declarations follow a consistent naming pattern and support a typical 3 masters + 3 replicas setup.
| tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true", | ||
| clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize environment variable checks across the codebase.
The environment variable checks use inconsistent patterns:
RATE_LIMIT_REDIS_TLS_DISABLED === "true"RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1"
This inconsistency could lead to confusion and potential errors.
Standardize the checks to use the same pattern across all Redis configuration files:
- tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true",
- clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",
+ tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "1",
+ clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true", | |
| clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", | |
| tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "1", | |
| clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", |
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | ||
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize environment variable checks (same issue as in realtimeClientGlobal.server.ts).
The environment variable checks use inconsistent patterns:
PUBSUB_REDIS_TLS_DISABLED === "true"PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1"
Standardize the checks:
- tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true",
- clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1",
+ tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "1",
+ clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | |
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", | |
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "1", | |
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", |
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | ||
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize environment variable checks (same issue as in other PubSub services).
The environment variable checks use inconsistent patterns:
PUBSUB_REDIS_TLS_DISABLED === "true"PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1"
Standardize the checks:
- tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true",
- clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1",
+ tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "1",
+ clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | |
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", | |
| tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "1", | |
| clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", |
| const nodes: ClusterNode[] = [ | ||
| { | ||
| host: options.host, | ||
| port: options.port, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cluster nodes configuration.
The cluster configuration assumes a single node, which doesn't fully utilize Redis cluster capabilities. Consider accepting multiple nodes through the options.
Enhance the cluster configuration:
- const nodes: ClusterNode[] = [
- {
- host: options.host,
- port: options.port,
- },
- ];
+ const nodes: ClusterNode[] = options.clusterOptions?.nodes ?? [
+ {
+ host: options.host,
+ port: options.port,
+ },
+ ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nodes: ClusterNode[] = [ | |
| { | |
| host: options.host, | |
| port: options.port, | |
| }, | |
| ]; | |
| const nodes: ClusterNode[] = options.clusterOptions?.nodes ?? [ | |
| { | |
| host: options.host, | |
| port: options.port, | |
| }, | |
| ]; |
| # redis-node-0: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-0 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6378:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-0:/bitnami/redis/data | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
|
|
||
| # redis-node-1: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-1 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6380:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-1:/bitnami/redis/data | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
|
|
||
| # redis-node-2: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-2 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6381:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-2:/bitnami/redis/data | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
|
|
||
| # redis-node-3: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-3 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6382:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-3:/bitnami/redis/data | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
|
|
||
| # redis-node-4: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-4 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6383:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-4:/bitnami/redis/data | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
|
|
||
| # redis-node-5: | ||
| # image: docker.io/bitnami/redis-cluster:7.0 | ||
| # container_name: redis-node-5 | ||
| # networks: | ||
| # - app_network | ||
| # ports: | ||
| # - "6384:6379" | ||
| # volumes: | ||
| # - redis-cluster_data-5:/bitnami/redis/data | ||
| # depends_on: | ||
| # - redis-node-0 | ||
| # - redis-node-1 | ||
| # - redis-node-2 | ||
| # - redis-node-3 | ||
| # - redis-node-4 | ||
| # environment: | ||
| # - "REDIS_PASSWORD=bitnami" | ||
| # - "REDISCLI_AUTH=bitnami" | ||
| # - "REDIS_CLUSTER_REPLICAS=1" | ||
| # - "REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5" | ||
| # - "REDIS_CLUSTER_CREATOR=yes" | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Documentation needed for Redis cluster configuration
The codebase lacks documentation for the Redis cluster setup. While there are commented service definitions in docker-compose.yml, there is no documentation explaining:
- When and how to enable the cluster mode
- The cluster setup process and configuration
- Prerequisites and manual steps needed
This documentation is important as Redis cluster setup requires specific steps and considerations for proper operation.
🔗 Analysis chain
Document the Redis cluster setup process.
The Redis cluster service definitions are well-structured but commented out. Consider adding documentation to:
- Explain when and how to enable the cluster mode
- Document the cluster setup process
- List any prerequisites or manual steps needed
Run this script to check if documentation exists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Redis cluster setup documentation
# Look for documentation files
echo "Searching for Redis cluster documentation..."
fd -t f -e md -e txt . | rg -i 'redis|cluster|setup|configuration'
Length of output: 208
Script:
#!/bin/bash
# Check content of Redis worker README
echo "Content of Redis worker README:"
cat internal-packages/redis-worker/README.md
Length of output: 406
Script:
#!/bin/bash
# Check for documentation in key locations
echo "=== Checking root README ==="
if [ -f "README.md" ]; then
cat README.md | rg -i 'redis|cluster|setup|configuration' -C 2
fi
echo -e "\n=== Checking docker directory ==="
if [ -f "docker/README.md" ]; then
cat docker/README.md
fi
echo -e "\n=== Checking docs directory ==="
fd -t f -e md -e txt . docs 2>/dev/null | xargs cat 2>/dev/null | rg -i 'redis|cluster|setup|configuration' -C 2
Length of output: 684
Script:
#!/bin/bash
# Check CONTRIBUTING.md for Redis setup instructions
echo "=== Checking CONTRIBUTING.md ==="
if [ -f "CONTRIBUTING.md" ]; then
cat CONTRIBUTING.md | rg -i 'redis|cluster|setup|configuration' -C 2
fi
Length of output: 1787
@trigger.dev/build
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
@trigger.dev/core
commit: |
Summary by CodeRabbit
New Features
Configuration Changes
Infrastructure