Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: add PubSub commands #2043

Merged
merged 7 commits into from
Jul 31, 2024
Merged

Conversation

shohamazon
Copy link
Collaborator

No description provided.

@shohamazon shohamazon requested a review from a team as a code owner July 29, 2024 08:44
@shohamazon shohamazon changed the title Core: add PubSub commands types Python: add PubSub commands Jul 29, 2024
@shohamazon shohamazon force-pushed the proto-pubsub branch 2 times, most recently from 98aae8d to 5ca1b6c Compare July 29, 2024 15:04
Signed-off-by: Shoham Elias <[email protected]>
See https://valkey.io/commands/pubsub-shardnumsub for details.

Args:
channels (Optional[List[str]]): The list of shard channels to query for the number of subscribers.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

channels type differs from the prototype

python/python/glide/async_commands/core.py Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments as the non-transaction

client = await create_client(request, cluster_mode)
# Assert no channels exists yet
assert await client.pubsub_channels() == []
await client.close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it important to close the client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure it shows like we have an open task (similar to what we had with node)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you have the cleanups towards the end of the func?

Copy link
Collaborator Author

@shohamazon shohamazon Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True it doesn't matter where the close is

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests are very good!

Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon shohamazon merged commit b9e7cef into valkey-io:main Jul 31, 2024
27 checks passed
@shohamazon shohamazon deleted the proto-pubsub branch July 31, 2024 16:16
"""
Returns the number of subscribers (exclusive of clients subscribed to patterns) for the specified shard channels.

Note that it is valid to call this command without channels. In this case, it will just return an empty map.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates Returns section

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem with that

"""
Returns the number of unique patterns that are subscribed to by clients.

Note: This is the total number of unique patterns all the clients are subscribed to,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "Notes:" followed by the list

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note is a verb here, not subject


Note: This is the total number of unique patterns all the clients are subscribed to,
not the count of clients subscribed to patterns.
The command is routed to all nodes, and aggregates the response the sum of all pattern subscriptions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing is not applicable to valkey's numpat command, so or remove that or describe how it became applicable there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need, to describe the whys here. it is described in the 'concepts' section of the docs

) -> List[bytes]:
"""
Lists the currently active channels.
The command is routed to all nodes, and aggregates the response to a single array.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing is not applicable to valkey's channels command, so or remove that or describe how it became applicable there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need. this is enough

Returns the number of subscribers (exclusive of clients subscribed to patterns) for the specified channels.

Note that it is valid to call this command without channels. In this case, it will just return an empty map.
The command is routed to all nodes, and aggregates the response to a single map of the channels and their number of subscriptions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing is not applicable to valkey's numsub command, so or remove that or describe how it became applicable there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

"""
Returns the number of subscribers (exclusive of clients subscribed to patterns) for the specified channels.

Note that it is valid to call this command without channels. In this case, it will just return an empty map.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates Returns section - remove

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem with that

"""
Returns the number of subscribers (exclusive of clients subscribed to patterns) for the specified channels.

Note that it is valid to call this command without channels. In this case, it will just return an empty map.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

@@ -2257,3 +2257,492 @@ async def test_pubsub_context_with_no_callback_raise_error(

with pytest.raises(ConfigurationError):
await create_two_clients_with_pubsub(request, cluster_mode, pub_sub_exact)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing cross slot tests for all commands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants