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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
