-
-
Notifications
You must be signed in to change notification settings - Fork 728
Support redis/valkey cluster mode #1650
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import { Cluster, Redis, type ClusterNode, type ClusterOptions } from "ioredis"; | ||
import { logger } from "./services/logger.server"; | ||
|
||
export type RedisWithClusterOptions = { | ||
host?: string; | ||
port?: number; | ||
username?: string; | ||
password?: string; | ||
tlsDisabled?: boolean; | ||
clusterMode?: boolean; | ||
clusterOptions?: Omit<ClusterOptions, "redisOptions">; | ||
keyPrefix?: string; | ||
}; | ||
|
||
export type RedisClient = Redis | Cluster; | ||
|
||
export function createRedisClient( | ||
connectionName: string, | ||
options: RedisWithClusterOptions | ||
): Redis | Cluster { | ||
if (options.clusterMode) { | ||
const nodes: ClusterNode[] = [ | ||
{ | ||
host: options.host, | ||
port: options.port, | ||
}, | ||
]; | ||
|
||
logger.debug("Creating a redis cluster client", { | ||
connectionName, | ||
host: options.host, | ||
port: options.port, | ||
}); | ||
|
||
return new Redis.Cluster(nodes, { | ||
...options.clusterOptions, | ||
redisOptions: { | ||
connectionName, | ||
keyPrefix: options.keyPrefix, | ||
username: options.username, | ||
password: options.password, | ||
enableAutoPipelining: true, | ||
...(options.tlsDisabled ? {} : { tls: {} }), | ||
}, | ||
}); | ||
} else { | ||
logger.debug("Creating a redis client", { | ||
connectionName, | ||
host: options.host, | ||
port: options.port, | ||
}); | ||
|
||
return new Redis({ | ||
connectionName, | ||
host: options.host, | ||
port: options.port, | ||
username: options.username, | ||
password: options.password, | ||
enableAutoPipelining: true, | ||
...(options.tlsDisabled ? {} : { tls: {} }), | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -12,8 +12,8 @@ function initializeRealtimeClient() { | |||||||||
host: env.RATE_LIMIT_REDIS_HOST, | ||||||||||
username: env.RATE_LIMIT_REDIS_USERNAME, | ||||||||||
password: env.RATE_LIMIT_REDIS_PASSWORD, | ||||||||||
enableAutoPipelining: true, | ||||||||||
...(env.RATE_LIMIT_REDIS_TLS_DISABLED === "true" ? {} : { tls: {} }), | ||||||||||
tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true", | ||||||||||
clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", | ||||||||||
Comment on lines
+15
to
+16
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. 🛠️ Refactor suggestion Standardize environment variable checks across the codebase. The environment variable checks use inconsistent patterns:
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
Suggested change
|
||||||||||
}, | ||||||||||
cachedLimitProvider: { | ||||||||||
async getCachedLimit(organizationId, defaultValue) { | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -25,8 +25,8 @@ function initializeDevPubSub() { | |||||||||
host: env.PUBSUB_REDIS_HOST, | ||||||||||
username: env.PUBSUB_REDIS_USERNAME, | ||||||||||
password: env.PUBSUB_REDIS_PASSWORD, | ||||||||||
enableAutoPipelining: true, | ||||||||||
...(env.PUBSUB_REDIS_TLS_DISABLED === "true" ? {} : { tls: {} }), | ||||||||||
tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | ||||||||||
clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", | ||||||||||
Comment on lines
+28
to
+29
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. 🛠️ Refactor suggestion Standardize environment variable checks (same issue as in realtimeClientGlobal.server.ts). The environment variable checks use inconsistent patterns:
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
Suggested change
|
||||||||||
}, | ||||||||||
schema: messageCatalog, | ||||||||||
}); | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,8 +26,8 @@ function initializeProjectPubSub() { | |||||||||
host: env.PUBSUB_REDIS_HOST, | ||||||||||
username: env.PUBSUB_REDIS_USERNAME, | ||||||||||
password: env.PUBSUB_REDIS_PASSWORD, | ||||||||||
enableAutoPipelining: true, | ||||||||||
...(env.PUBSUB_REDIS_TLS_DISABLED === "true" ? {} : { tls: {} }), | ||||||||||
tlsDisabled: env.PUBSUB_REDIS_TLS_DISABLED === "true", | ||||||||||
clusterMode: env.PUBSUB_REDIS_CLUSTER_MODE_ENABLED === "1", | ||||||||||
Comment on lines
+29
to
+30
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. 🛠️ Refactor suggestion Standardize environment variable checks (same issue as in other PubSub services). The environment variable checks use inconsistent patterns:
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
Suggested change
|
||||||||||
}, | ||||||||||
schema: messageCatalog, | ||||||||||
}); | ||||||||||
|
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.
🛠️ 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:
📝 Committable suggestion