Handle ASK redirects to slotless nodes during resharding - #4214
Conversation
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for picking this up, and for the precise scoping — the diagnosis is right. initialize() builds nodes_cache purely from CLUSTER SLOTS, so a slotless importing node is never in it and the ASK retry dereferences None in both stacks. That part of the fix is the direction we want.
Before we can merge it, the redirect target needs an owner. As written, the temporary ClusterNode is intentionally kept out of nodes_cache and startup_nodes, which are the only collections aclose() / set_nodes(remove_old=True) ever tear down, so every ASK-redirected command leaves a connection behind. On the async side ClusterNode.__del__ can only warn ("Unclosed ClusterNode object") — it cannot close the transport. On the sync side each redirect builds a fresh Redis client, connection pool, and handshake via create_redis_connections(). During a real reshard that is one orphaned connection per redirected command, in exactly the sustained-traffic scenario the issue describes.
Could you please:
- make the ad-hoc node manager-owned and short-lived — reused for repeated redirects to the same address, and disconnected once it is no longer needed;
- route the redirect address through
remap_host_port()soaddress_remapusers are covered; - extend the tests so they exercise real connection acquisition and release and assert that nothing is left behind after the command. The current tests patch out
get_redis_connection/ClusterNode.execute_command, so they prove theNonedereference is gone but not the resource behavior.
A rebase is also needed — the branch currently conflicts with master — and the PR is still a draft. Happy to review again once those are in.
eeshsaxena
left a comment
There was a problem hiding this comment.
The slotless-node handling looks right. On the sync side ClusterNode(e.host, e.port, PRIMARY) is created with redis_connection=None, and the first time it is used get_redis_connection -> nodes_manager.create_redis_connections([node]) -> create_redis_node(..., **self.connection_kwargs), so TLS/auth/timeout config is still applied even though the node was built ad hoc for the ASK target. The async side passes **self.nodes_manager.connection_kwargs at construction since the async ClusterNode owns its own pool. Different mechanism, same outcome: both inherit the cluster's connection settings.
This also fixes the previous behavior where get_node(node_name=...) returned None for a node not in the slot map, and the next loop iteration would call execute_command("ASKING") on None. Looks good.
Fixes #4015
What changed
ClusterNodeonly when the redirect target is unknown.ASKING, then retry the original command.nodes_cache.Verification
pytest -q tests/test_cluster.py -m onlycluster -k 'ask_redirection_to_node_without_slots'— 1 passedpytest -q tests/test_asyncio/test_cluster.py -k 'ask_redirection_to_node_without_slots'— 1 passedinvoke lintersruff format --checkgit diff --checkpython -m compileallThe full Redis Cluster integration suite requires local Redis cluster services, which are not available in this workspace.