You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2026/08/04 07:39:41 UTC
[PR] Add Redis cluster mode support to RedisHook [airflow]
FrankYang0529 opened a new pull request, #71067:
URL: https://github.com/apache/airflow/pull/71067
## Why
- Redis Cluster spreads the keyspace over 16384 hash slots and expects the client to route each command to the node owning that slot. A standalone client does not do this — the node replies `MOVED` and the command fails.
## How
- Add a `cluster` connection extra. When set, `get_conn()` builds a `redis.cluster.RedisCluster`, which follows `MOVED`/`ASK` redirects.
- Add a `startup_nodes` extra to supply extra bootstrap nodes.
## Verification
- `uv run --project providers/redis pytest providers/redis/tests/unit/redis/hooks/test_redis.py -xvs`
- `breeze testing providers-integration-tests --db-reset --integration redis`
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
* closes: #10504
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [X] Yes - Claude Code
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3796779218
##########
providers/redis/docs/connections.rst:
##########
@@ -31,22 +32,30 @@ parameter as ``redis_default`` by default.
Configuring the Connection
--------------------------
Host
- The host of the Redis cluster.
+ The host of the Redis server.
Port
- Specify the port to use for connecting the Redis cluster (Default is ``6379``).
+ Specify the port to use for connecting the Redis server (Default is ``6379``).
Login
- The user that will be used for authentication against the Redis cluster (only applicable in Redis 6.0 and above).
+ The user that will be used for authentication against the Redis server (only applicable in Redis 6.0 and above).
Password
- The password of the user that will be used for authentication against the Redis cluster.
+ The password of the user that will be used for authentication against the Redis server.
DB
- The DB number to use in the Redis cluster (Default is ``0``).
+ The DB number to use in the Redis server (Default is ``0``). Not supported in cluster mode.
+
+Enable cluster mode
+ Whether to connect with a cluster-aware client that follows ``MOVED``/``ASK`` redirects
+ (Default is ``False``). See :ref:`redis-cluster-mode` below.
Review Comment:
Thanks for the great suggestion. Yes, the `Is cluster` is more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3801212101
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,49 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ entries = self.startup_nodes.split(",") if isinstance(self.startup_nodes, str) else self.startup_nodes
Review Comment:
Thanks for the review and suggestion. Add value verification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3796783095
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -76,6 +84,14 @@ def get_conn(self):
self.username = conn.login
self.password = None if str(conn.password).lower() in ["none", "false", ""] else conn.password
self.db = conn.extra_dejson.get("db")
+ self.cluster = conn.extra_dejson.get("cluster", False)
Review Comment:
Updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3855285433
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,54 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ if not isinstance(self.startup_nodes, str):
+ raise ValueError(
+ "`startup_nodes` must be a comma-separated string of `host:port` entries, got "
Review Comment:
```suggestion
"The `startup_nodes` parameter value must be a comma-separated string of `host:port` entries, got "
```
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,54 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ if not isinstance(self.startup_nodes, str):
+ raise ValueError(
+ "`startup_nodes` must be a comma-separated string of `host:port` entries, got "
+ f"{self.startup_nodes!r}."
+ )
+
+ nodes = []
+ for entry in self.startup_nodes.split(","):
+ host, _, port = entry.strip().partition(":")
+ if not host:
+ raise ValueError(f"Missing host in `startup_nodes` entry {entry!r}; expected `host:port`.")
Review Comment:
```suggestion
raise ValueError(f"Missing host in `startup_nodes` parameter value for entry {entry!r}; expected `host:port`.")
```
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,54 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ if not isinstance(self.startup_nodes, str):
+ raise ValueError(
+ "`startup_nodes` must be a comma-separated string of `host:port` entries, got "
+ f"{self.startup_nodes!r}."
+ )
+
+ nodes = []
+ for entry in self.startup_nodes.split(","):
+ host, _, port = entry.strip().partition(":")
+ if not host:
+ raise ValueError(f"Missing host in `startup_nodes` entry {entry!r}; expected `host:port`.")
+ try:
+ parsed_port = int(port) if port else DEFAULT_REDIS_PORT
+ except ValueError:
+ raise ValueError(
+ f"Invalid port in `startup_nodes` entry {entry!r}; expected `host:port`."
Review Comment:
```suggestion
f"Invalid port in `startup_nodes` parameter value for entry {entry!r}; expected `host:port`."
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3792222544
##########
providers/redis/docs/connections.rst:
##########
@@ -31,22 +32,30 @@ parameter as ``redis_default`` by default.
Configuring the Connection
--------------------------
Host
- The host of the Redis cluster.
+ The host of the Redis server.
Port
- Specify the port to use for connecting the Redis cluster (Default is ``6379``).
+ Specify the port to use for connecting the Redis server (Default is ``6379``).
Login
- The user that will be used for authentication against the Redis cluster (only applicable in Redis 6.0 and above).
+ The user that will be used for authentication against the Redis server (only applicable in Redis 6.0 and above).
Password
- The password of the user that will be used for authentication against the Redis cluster.
+ The password of the user that will be used for authentication against the Redis server.
DB
- The DB number to use in the Redis cluster (Default is ``0``).
+ The DB number to use in the Redis server (Default is ``0``). Not supported in cluster mode.
+
+Enable cluster mode
+ Whether to connect with a cluster-aware client that follows ``MOVED``/``ASK`` redirects
+ (Default is ``False``). See :ref:`redis-cluster-mode` below.
Review Comment:
```suggestion
Is cluster
Whether Redis deployment is a cluster or a standalone instance (Default is ``False``). See :ref:`redis-cluster-mode` below.
```
I think we should make the description a bit simpler - we have more in-detail explanation in the mode doc. `Is cluster` instead of `Enable cluster mode`, as this setting represents what redis to which we connect to, rather than configuring Redis itself.
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -141,6 +189,14 @@ def get_connection_form_widgets(cls) -> dict[str, Any]:
return {
"db": IntegerField(lazy_gettext("DB"), widget=BS3TextFieldWidget(), default=0),
+ "cluster": BooleanField(lazy_gettext("Enable cluster mode"), default=False),
Review Comment:
```suggestion
"cluster": BooleanField(lazy_gettext("Is cluster"), default=False),
```
##########
providers/redis/src/airflow/providers/redis/get_provider_info.py:
##########
@@ -65,6 +65,15 @@ def get_provider_info():
"ui-field-behaviour": {"hidden-fields": ["schema", "extra"], "relabeling": {}},
"conn-fields": {
"db": {"label": "DB", "schema": {"type": ["integer", "null"], "default": 0}},
+ "cluster": {
+ "label": "Enable cluster mode",
+ "schema": {"type": ["boolean", "null"], "default": False},
+ },
+ "startup_nodes": {
+ "label": "Cluster startup nodes",
+ "description": "Comma-separated extra bootstrap nodes as host:port. Cluster mode only.",
Review Comment:
```suggestion
"description": "Comma-separated extra bootstrap nodes as host:port. Only for cluster Redis deployments.",
```
##########
providers/redis/docs/connections.rst:
##########
@@ -31,22 +32,30 @@ parameter as ``redis_default`` by default.
Configuring the Connection
--------------------------
Host
- The host of the Redis cluster.
+ The host of the Redis server.
Port
- Specify the port to use for connecting the Redis cluster (Default is ``6379``).
+ Specify the port to use for connecting the Redis server (Default is ``6379``).
Login
- The user that will be used for authentication against the Redis cluster (only applicable in Redis 6.0 and above).
+ The user that will be used for authentication against the Redis server (only applicable in Redis 6.0 and above).
Password
- The password of the user that will be used for authentication against the Redis cluster.
+ The password of the user that will be used for authentication against the Redis server.
DB
- The DB number to use in the Redis cluster (Default is ``0``).
+ The DB number to use in the Redis server (Default is ``0``). Not supported in cluster mode.
+
+Enable cluster mode
+ Whether to connect with a cluster-aware client that follows ``MOVED``/``ASK`` redirects
+ (Default is ``False``). See :ref:`redis-cluster-mode` below.
+
+Cluster startup nodes
Review Comment:
```suggestion
Startup nodes
```
To be consistent with other places within the code.
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -76,6 +84,14 @@ def get_conn(self):
self.username = conn.login
self.password = None if str(conn.password).lower() in ["none", "false", ""] else conn.password
self.db = conn.extra_dejson.get("db")
+ self.cluster = conn.extra_dejson.get("cluster", False)
Review Comment:
We could get `startup_nodes` here too and use it directly in `_build_startup_nodes` method.
##########
providers/redis/docs/connections.rst:
##########
@@ -64,3 +73,26 @@ Certificate path
Enable hostname check
If set, match the hostname during the SSL handshake (Default is ``False``).
+
+.. _redis-cluster-mode:
+
+Cluster mode
+------------
+
+Redis Cluster spreads the keyspace over 16384 hash slots owned by different masters, and expects
+the client to route each command to the node owning that key's slot. A standalone client does not
+do this: when it asks a node for a key that node does not serve, the node answers ``MOVED`` and
+the standalone client fails.
+
+Enable cluster mode to use a cluster-aware client that follows those redirects:
+
+.. code-block:: json
+
+ {
+ "cluster": true,
+ "startup_nodes": ["node-2:6379", "node-3:6379"]
Review Comment:
```suggestion
"startup_nodes": ["node-2:6379", "node-3:6379"] # Connection extra
```
It will be easier for user to know where this setting can be set.
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -45,6 +47,11 @@ class RedisHook(BaseHook):
You can set your db in the extra field of your connection as ``{"db": 3}``.
Also you can set ssl parameters as:
``{"ssl": true, "ssl_cert_reqs": "require", "ssl_certfile": "/path/to/cert.pem", etc}``.
+
+ To talk to a Redis deployment running in cluster mode, set ``{"cluster": true}``. Additional
+ bootstrap nodes may be listed as ``{"startup_nodes": ["node-2:6379", "node-3:6379"]}`` so that
Review Comment:
```suggestion
bootstrap nodes may be listed as Connection Extras ``{"startup_nodes": ["node-2:6379", "node-3:6379"]}`` so that
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3796781385
##########
providers/redis/docs/connections.rst:
##########
@@ -64,3 +73,26 @@ Certificate path
Enable hostname check
If set, match the hostname during the SSL handshake (Default is ``False``).
+
+.. _redis-cluster-mode:
+
+Cluster mode
+------------
+
+Redis Cluster spreads the keyspace over 16384 hash slots owned by different masters, and expects
+the client to route each command to the node owning that key's slot. A standalone client does not
+do this: when it asks a node for a key that node does not serve, the node answers ``MOVED`` and
+the standalone client fails.
+
+Enable cluster mode to use a cluster-aware client that follows those redirects:
+
+.. code-block:: json
+
+ {
+ "cluster": true,
+ "startup_nodes": ["node-2:6379", "node-3:6379"]
Review Comment:
Change to use `// Connection extra`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3858658321
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,54 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ if not isinstance(self.startup_nodes, str):
+ raise ValueError(
+ "`startup_nodes` must be a comma-separated string of `host:port` entries, got "
Review Comment:
Thanks for the review. Updated all three suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3841821653
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,49 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ entries = self.startup_nodes.split(",") if isinstance(self.startup_nodes, str) else self.startup_nodes
Review Comment:
Sorry for the late reply. After rechecking the design, I think we don't need list type here. We can just use comma-separated string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3814582567
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,49 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ entries = self.startup_nodes.split(",") if isinstance(self.startup_nodes, str) else self.startup_nodes
Review Comment:
Is there some reason why we would want to have two value formats supported instead of one? 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #71067:
URL: https://github.com/apache/airflow/pull/71067#issuecomment-5316751102
@Miretpl Thanks for the review. I addressed all comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #71067:
URL: https://github.com/apache/airflow/pull/71067#discussion_r3798300640
##########
providers/redis/src/airflow/providers/redis/hooks/redis.py:
##########
@@ -111,18 +130,49 @@ def get_conn(self):
"lib_name": f"redis-py(apache-airflow-providers-redis_v{provider_version})",
}
- self.redis = Redis(
- host=self.host,
- port=self.port,
- username=self.username,
- password=self.password,
- db=self.db,
- **ssl_args,
- **driver_info_options,
- )
+ if self.cluster:
+ self.redis = RedisCluster(
+ host=self.host,
+ port=self.port,
+ startup_nodes=self._build_startup_nodes(),
+ username=self.username,
+ password=self.password,
+ **ssl_args,
+ **driver_info_options,
+ )
+ else:
+ self.redis = Redis(
+ host=self.host,
+ port=self.port,
+ username=self.username,
+ password=self.password,
+ db=self.db,
+ **ssl_args,
+ **driver_info_options,
+ )
return self.redis
+ def _build_startup_nodes(self) -> list[ClusterNode]:
+ """Build redis-py cluster nodes from the ``startup_nodes`` extra, given as ``host`` or ``host:port``."""
+ if not self.startup_nodes:
+ return []
+
+ entries = self.startup_nodes.split(",") if isinstance(self.startup_nodes, str) else self.startup_nodes
Review Comment:
Maybe instead of assuming what the user will enter, we should verify values and raise an exception if they are not, e.g. a list of strings?
##########
providers/redis/docs/connections.rst:
##########
@@ -64,3 +73,26 @@ Certificate path
Enable hostname check
If set, match the hostname during the SSL handshake (Default is ``False``).
+
+.. _redis-cluster-mode:
+
+Cluster mode
Review Comment:
```suggestion
Redis Cluster Deployment
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on PR #71067:
URL: https://github.com/apache/airflow/pull/71067#issuecomment-5308621543
Could you also adjust the PR description in terms of why it is needed? Currently, we only know the difference between standalone Redis and cluster deployment and how it handles connections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Redis cluster mode support to RedisHook [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl merged PR #71067:
URL: https://github.com/apache/airflow/pull/71067
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org