support json() +with pipeline in asyncio.cluster mode #2423

nlsj1985 · 2022-10-20T23:03:46Z

Pull Request check-list

Please make sure to review and check all of these items:

Does $ tox pass with this change (including linting)?
Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Is there an example added to the examples folder (if applicable)?
Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please provide a description of the change here.

Hi, as newbie redis-user I ran into #2234 with asyncio cluster support for json() + also in combination with pipelines
I've created some patches, that resulted in functioning support for this.
As i'm not practiced enough with big projects like these, I would like to request someone to review the changes that I've made. I might have broken some other things (or not), and I'm sure there are some rookie mistakes in the changes.. but it works for me.. and I've spent quite some hours on this.
So I hope someone who's more experienced could help pickup my improvements for the project, and improve upon it.
I'm guessing there also needs a bit of testing to get extended.
I unfortunately have no resources to complete that, and run the complete set op tests, and to investigate the results.
I've ran my test.py (see examples/) using python3.10.4 and the docker image redis-modcluster and that works fine for my usecase (looking from the surface, in my brief tests), I unfortunately have to carry-on doing other stuff.
Thanks in advance!

…line (for json())

utkarshgupta137

I think you don't need any changes to cluster.py except for adding AsyncRedisModuleCommands inheritance to the RedisCluster & ClusterPipeline classes.

utkarshgupta137 · 2022-10-20T23:35:25Z

redis/asyncio/cluster.py

@@ -298,7 +300,10 @@ def __init__(
            # Call our on_connect function to configure READONLY mode
            kwargs["redis_connect_func"] = self.on_connect

-        kwargs["response_callbacks"] = self.__class__.RESPONSE_CALLBACKS.copy()
+        kwargs["cluster_response_callbacks"] = CaseInsensitiveDict(self.__class__.RESPONSE_CALLBACKS)


What is the purpose of renaming this variable? Also, what is the advantage of using CaseInsensitiveDict? It is significantly slower that a normal dict due to the case conversion for each call.

utkarshgupta137 · 2022-10-20T23:38:11Z

redis/asyncio/cluster.py

@@ -1206,7 +1211,7 @@ async def close(self, attr: str = "nodes_cache") -> None:
        )


-class ClusterPipeline(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommands):
+class ClusterPipeline(RedisCluster):


Why are the bases for this class changed? Inheriting directly from the RedisCluster class could lead to collisions in the namespace

utkarshgupta137 · 2022-10-20T23:39:48Z

redis/asyncio/cluster.py

+    def __init__(
+            self,
+            client: RedisCluster,
+            nodes_manager=None,


The new kwargs are not used anywhere?

utkarshgupta137 · 2022-10-20T23:40:43Z

redis/asyncio/cluster.py

@@ -298,7 +300,10 @@ def __init__(
            # Call our on_connect function to configure READONLY mode
            kwargs["redis_connect_func"] = self.on_connect

-        kwargs["response_callbacks"] = self.__class__.RESPONSE_CALLBACKS.copy()
+        kwargs["cluster_response_callbacks"] = CaseInsensitiveDict(self.__class__.RESPONSE_CALLBACKS)
+        self.cluster_response_callbacks = CaseInsensitiveDict(


This will always get overwritten on line 332.

utkarshgupta137 · 2022-10-20T23:41:34Z

redis/asyncio/cluster.py

        self._client = client
-
+        self.cluster_response_callbacks = self.RESPONSE_CALLBACKS


This is not used anywhere?

utkarshgupta137 · 2022-10-20T23:44:26Z

redis/asyncio/cluster.py

@@ -152,6 +153,7 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
        - none of the `host`/`port` & `startup_nodes` were provided

    """
+    response_callbacks: MutableMapping[Union[str, bytes], ResponseCallbackT]


This is defined but never initialized?

nlsj1985 · 2022-10-21T08:40:48Z

@utkarshgupta137 I was hopping you would check this out and provide pointers or improvements. As said, i'm really inexperienced with all this and hoped to help out.. but i was really struggling by comparing sync cluster, pipeline and json() with the current implemented async code, so just started copying stuff, run it, and patching the next exception in any way I could think of, till I got it working and then I was drained and confused by how things got to the point where it started to work ;-)

nlsj1985 · 2022-10-21T09:30:44Z

I've reverted quite some changes and took your recommendations
cluster.py:
class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommands, AsyncRedisModuleCommands):
class ClusterPipeline(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommands, AsyncRedisModuleCommands):
def init(self, client: RedisCluster) -> None:
super().init() # because my IDE say init needed the super class initialized
self._client = client
self._command_stack: List["PipelineCommand"] = []

resulting in:
File "C:\Users\janse499\PycharmProjects\redis-py\redis\commands\redismodules.py", line 110, in json
jj = AsyncJSON(client=self, encoder=encoder, decoder=decoder)
File "C:\Users\janse499\PycharmProjects\redis-py\redis\commands\json_init_.py", line 65, in init
self.client.set_response_callback(key, value)
AttributeError: 'ClusterPipeline' object has no attribute 'set_response_callback'

set_response_callback() was used before from inheriting RedisCluster..
I've been looking a bit again over client.py and cluster.py and the different pipelines, and trying some stuff.. but this is to complex for me to fix at this moment. I hope you might have some more pointers, I'm happy to learn.

ps. also, when I removed RedisCluster from ClusterPipeline's inheritance the json_init_.py is not accepting the parameters in AsyncioClusterPipeline() anymore at line 178

KR

utkarshgupta137 · 2022-10-22T13:28:05Z

I've never used any redis modules, so I can't work on this myself. I can review cluster related code though. For now, I would recommend not changing anything in the cluster.py file except adding inheritance for async redis modules command to the client & pipeline classes.

nlsj1985 · 2022-10-23T13:46:10Z

If you've docker running somewhere you could run a redis cluster with modules using:
docker run -p 46379:46379/tcp -p 46380:46380/tcp -p 46381:46381/tcp -v /<some path>/redismod_cluster/redis.conf:/redis.conf --name redis-modcluster redisfab/redis-py-modcluster:6.2.6

Then you can use the test.py that i've added in the example dir once you've made and build changes.

note. running the redis-cluster with modules makes my NVME drive make wierd sounds and memory availability limited

github-actions · 2023-10-24T00:22:45Z

This pull request is marked stale. It will be closed in 30 days if it is not updated.

nlsj1985 added 2 commits October 21, 2022 00:16

get json() from modules working in asyncio.cluster + AsyncClusterPipe…

01c76e8

…line (for json())

added test.py as an example of async cluster pipeline json commands

fa5c4ac

utkarshgupta137 suggested changes Oct 20, 2022

View reviewed changes

the-wondersmith mentioned this pull request May 18, 2023

Add support for JSON commands in Async Cluster #2234

Closed

github-actions bot added the Stale label Oct 24, 2023

github-actions bot closed this Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support json() +with pipeline in asyncio.cluster mode #2423

support json() +with pipeline in asyncio.cluster mode #2423

nlsj1985 commented Oct 20, 2022

utkarshgupta137 left a comment •

edited

Loading

utkarshgupta137 Oct 20, 2022

utkarshgupta137 Oct 20, 2022

utkarshgupta137 Oct 20, 2022 •

edited

Loading

utkarshgupta137 Oct 20, 2022

utkarshgupta137 Oct 20, 2022

utkarshgupta137 Oct 20, 2022

nlsj1985 commented Oct 21, 2022

nlsj1985 commented Oct 21, 2022

utkarshgupta137 commented Oct 22, 2022

nlsj1985 commented Oct 23, 2022

github-actions bot commented Oct 24, 2023

		self._client = client

		self.cluster_response_callbacks = self.RESPONSE_CALLBACKS

support json() +with pipeline in asyncio.cluster mode #2423

support json() +with pipeline in asyncio.cluster mode #2423

Conversation

nlsj1985 commented Oct 20, 2022

Pull Request check-list

Description of change

utkarshgupta137 left a comment • edited Loading

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022

Choose a reason for hiding this comment

utkarshgupta137 Oct 20, 2022

Choose a reason for hiding this comment

nlsj1985 commented Oct 21, 2022

nlsj1985 commented Oct 21, 2022

utkarshgupta137 commented Oct 22, 2022

nlsj1985 commented Oct 23, 2022

github-actions bot commented Oct 24, 2023

utkarshgupta137 left a comment •

edited

Loading

utkarshgupta137 Oct 20, 2022 •

edited

Loading