Python: enable routing by node address. #1038

nihohit · 2024-02-26T08:50:43Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ikolomi · 2024-02-26T09:48:33Z

python/python/glide/routes.py

@@ -48,6 +48,18 @@ def __init__(self, slot_type: SlotType, slot_id: int) -> None:
        self.slot_id = slot_id


+class ByAddressRoute(Route):
+    def __init__(self, host: str, port: Optional[int] = None) -> None:


we need to specify the format of the host (ip:tcp_port) and handle erroneous values, or make host -> host_ip.
Document that if port is None than a default will be used

barshaul · 2024-02-26T13:23:13Z

python/python/glide/routes.py

@@ -48,6 +48,24 @@ def __init__(self, slot_type: SlotType, slot_id: int) -> None:
        self.slot_id = slot_id


+class ByAddressRoute(Route):
+    def __init__(self, host: str, port: Optional[int] = None) -> None:
+        """Routes a request to a node by its DNS address


I don't think DNS is right, as it can be also IP.
maybe change to

Routes a request to a node based on its host address and IP

when/how can it be IP? I'm not sure that it will work. Also, why would the user have the IP of the machine?

how will we support when nodes dont have the dns but only the IP ?

🤷
This PR aligns the python API with the node API. AFAIK we don't support connecting using an IP, so I'm not sure that just passing the IP as a string will work. I recommend spinning off IP support in routing to a separate PR, since I believe it will require core & redis-rs work.

I think it depends on how user configured the cluster. If nodes know each other by IP, so response of cluster slots would contain IPs only and glide core would know IPs only.
Probably, we need to refer there to cluster configuration, like

Suggested change

"""Routes a request to a node by its DNS address

"""Routes a request to a node by its address given on cluster configuration

when/how can it be IP? I'm not sure that it will work. Also, why would the user have the IP of the machine?

If the cluster is configured with TLS disabled, the nodes will be shown in cluster slots/nodes with their IPs, rather than their DNS. Yuri is right that it depends on server.

barshaul · 2024-02-26T13:23:32Z

python/python/glide/routes.py

+        """Routes a request to a node by its DNS address
+
+        Args:
+            host (str): The DNS name of the node. If `port` is not provided, should be in the `{DNS}:{port}` format.


same regarding DNS

same as my comment above - should we support IP host addresses also?

barshaul · 2024-02-26T13:25:30Z

python/python/glide/routes.py

+        if port == None:
+            split = host.split(":")
+            host = split[0]
+            port = int(split[1])


It's a potential error, we should catch this exception and raise appropriate client error

May be use a 3rd lib which does all checks for us?
Consider that a user may give two different ports or port out of range.

barshaul · 2024-02-26T13:26:34Z

python/python/glide/routes.py

+            port = int(split[1])
+
+        self.host = host
+        self.port = port if port else 0


How can port be None at this point?

Ask mypy. I only added this to satisfy it 🤷 .

then lets do something like:

if port is None: split = host.split(":") self.host = split[0] self.port = int(split[1]) else: self.host = host self.port = port

barshaul · 2024-02-26T13:26:56Z

python/python/glide/routes.py

+            port (Optional[int]): The port the node communicates on.
+        """
+        super().__init__()
+        if port == None:


if port is None

barshaul · 2024-02-26T13:30:27Z

python/python/tests/test_async_client.py

@@ -1416,6 +1417,36 @@ async def test_info_random_route(self, redis_client: RedisClusterClient):
        assert isinstance(info, str)
        assert "# Server" in info

+    @pytest.mark.parametrize("cluster_mode", [True])


what about the test for failing if port isn't passed?

nihohit · 2024-02-26T16:44:48Z

@barshaul @ikolomi answered / fixed comments.

barshaul · 2024-03-04T07:33:55Z

node/src/RedisClusterClient.ts

@@ -56,7 +57,7 @@ export type SlotKeyTypes = {
 export type RouteByAddress = {
    type: "routeByAddress";
    /**
-     * DNS name of the host.
+     * The name of the node. If `port` is not provided, should be in the `{DNS/ip}:{port}` format, as returned from the cluster when sending a request to multiple nodes.


I don't think it's a good way of explaining it, as the results of some commands that are being sent to multiple nodes are being aggregated to a single response.. so the user really needs to know the client to understand that. Can you check if we can use cluster nodes/slots output to explain what should be passed?

barshaul · 2024-03-04T07:35:52Z

python/python/glide/routes.py

@@ -48,6 +49,29 @@ def __init__(self, slot_type: SlotType, slot_id: int) -> None:
        self.slot_id = slot_id


+class ByAddressRoute(Route):
+    def __init__(self, host: str, port: Optional[int] = None) -> None:
+        """Routes a request to a node by its DNS address


Again- "DNS" isn't right here and in the documentation below

barshaul · 2024-03-04T07:37:33Z

python/python/glide/routes.py

+            if len(split) < 2:
+                raise RequestError(
+                    "No port provided, expected host to be formatted as {hostname}:{port}`. Received "
+                    + host


Minor: use f"...{host}"

Yury-Fridlyand

Please, add a UT to cover parsing error cases, like address line is "abc:def"
Please, add an IT with wrong address and/or port

node/src/RedisClusterClient.ts

python/python/glide/routes.py

Co-authored-by: Yury-Fridlyand <[email protected]>

* Python: enable routing by node address. * Added correct error handling to the split. * Fix check * fix * Updated host's documentation. * fix docs * Update python/python/glide/routes.py Co-authored-by: Yury-Fridlyand <[email protected]> * Update node/src/RedisClusterClient.ts Co-authored-by: Yury-Fridlyand <[email protected]> --------- Co-authored-by: Shachar Langbeheim <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]>

nihohit requested a review from a team as a code owner February 26, 2024 08:50

shachlanAmazon requested review from barshaul, shohamazon, ikolomi and adanWattad February 26, 2024 08:51

nihohit force-pushed the python-address-routing branch from 6578a66 to 0cb5414 Compare February 26, 2024 09:41

ikolomi reviewed Feb 26, 2024

View reviewed changes

nihohit force-pushed the python-address-routing branch 2 times, most recently from ef1914b to 32edaaa Compare February 26, 2024 10:43

barshaul reviewed Feb 26, 2024

View reviewed changes

Yury-Fridlyand added the python Python wrapper label Feb 26, 2024

barshaul requested changes Mar 4, 2024

View reviewed changes

barshaul approved these changes Mar 6, 2024

View reviewed changes

nihohit force-pushed the python-address-routing branch from debba53 to d42f109 Compare March 6, 2024 16:40

Yury-Fridlyand approved these changes Mar 6, 2024

View reviewed changes

node/src/RedisClusterClient.ts Outdated Show resolved Hide resolved

python/python/glide/routes.py Outdated Show resolved Hide resolved

nihohit and others added 8 commits March 6, 2024 17:39

Python: enable routing by node address.

0cf25fb

Added correct error handling to the split.

b007015

Fix check

f77e458

fix

5fc078a

Updated host's documentation.

52c14a8

fix docs

168d7f8

Update python/python/glide/routes.py

3e0754d

Co-authored-by: Yury-Fridlyand <[email protected]>

Update node/src/RedisClusterClient.ts

13dc9ca

Co-authored-by: Yury-Fridlyand <[email protected]>

nihohit force-pushed the python-address-routing branch from b018c97 to 13dc9ca Compare March 6, 2024 17:39

shachlanAmazon merged commit 25a30bd into valkey-io:main Mar 6, 2024
11 checks passed

nihohit deleted the python-address-routing branch March 6, 2024 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: enable routing by node address. #1038

Python: enable routing by node address. #1038

nihohit commented Feb 26, 2024

ikolomi Feb 26, 2024

barshaul Feb 26, 2024

nihohit Feb 26, 2024

ikolomi Feb 26, 2024

nihohit Feb 26, 2024

Yury-Fridlyand Feb 26, 2024

barshaul Feb 28, 2024 •

edited

Loading

barshaul Feb 26, 2024

ikolomi Feb 26, 2024

barshaul Feb 26, 2024

Yury-Fridlyand Feb 26, 2024

barshaul Feb 26, 2024

nihohit Feb 26, 2024

barshaul Feb 28, 2024

barshaul Feb 26, 2024

barshaul Feb 26, 2024

nihohit commented Feb 26, 2024

barshaul Mar 4, 2024

barshaul Mar 4, 2024

barshaul Mar 4, 2024

Yury-Fridlyand left a comment •

edited

Loading

	"""Routes a request to a node by its DNS address
	"""Routes a request to a node by its address given on cluster configuration

Python: enable routing by node address. #1038

Python: enable routing by node address. #1038

Conversation

nihohit commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihohit commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment • edited Loading

Choose a reason for hiding this comment

barshaul Feb 28, 2024 •

edited

Loading

Yury-Fridlyand left a comment •

edited

Loading