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 Script commands #2208

Merged
merged 11 commits into from
Sep 9, 2024

Conversation

tjzhang-BQ
Copy link
Collaborator

@tjzhang-BQ tjzhang-BQ commented Aug 28, 2024

Inheriting the changes from PR: #2094 & addressing existing comments

putting transactions out of scope for this PR since script_invocation and script_invocation_pointers seem to be parallel with transactions and more investigation might be needed for the implementation of invoke_script.

@tjzhang-BQ tjzhang-BQ added the python Python wrapper label Aug 28, 2024
@tjzhang-BQ tjzhang-BQ force-pushed the python/VALKEY-187-script-flush branch 2 times, most recently from 78ef000 to a735a8d Compare September 3, 2024 23:48
@tjzhang-BQ tjzhang-BQ marked this pull request as ready for review September 4, 2024 00:37
@tjzhang-BQ tjzhang-BQ requested a review from a team as a code owner September 4, 2024 00:37
python/python/glide/async_commands/cluster_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Outdated Show resolved Hide resolved
python/python/glide/async_commands/standalone_commands.py Outdated Show resolved Hide resolved
python/python/tests/test_async_client.py Show resolved Hide resolved
python/python/tests/test_async_client.py Outdated Show resolved Hide resolved
python/python/tests/utils/utils.py Outdated Show resolved Hide resolved
@tjzhang-BQ tjzhang-BQ force-pushed the python/VALKEY-187-script-flush branch 3 times, most recently from b2b699b to 2cd39fe Compare September 5, 2024 00:58
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Transaction?

CHANGELOG.md Outdated Show resolved Hide resolved
case the client will route the command to the nodes defined by `route`. Defaults to None.

Returns:
List[bool]: A list of boolean values indicating the existence of each script.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
List[bool]: A list of boolean values indicating the existence of each script.
TClusterResponse[List[bool]]: A list of boolean values indicating the existence of each script.
When specifying a route other than a single node, response will be:
{Address (bytes) : response (List[bool]) , ... } with type of Dict[bytes, List[bool]].

Copy link
Collaborator

Choose a reason for hiding this comment

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

are we sure this is the response type? when you specify all nodes ?

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 believe if we specify to all nodes, the results across different nodes are just aggregated using a logical AND, according to cluster_routing.rs in redis.rs, and still returns a list of booleans.

Copy link
Collaborator

@jonathanl-bq jonathanl-bq left a comment

Choose a reason for hiding this comment

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

Spotted some small documentation mistakes. Please fix those first.

Signed-off-by: TJ Zhang <[email protected]>
@tjzhang-BQ tjzhang-BQ merged commit 986b8c9 into valkey-io:main Sep 9, 2024
41 checks passed
@tjzhang-BQ tjzhang-BQ deleted the python/VALKEY-187-script-flush branch September 9, 2024 20:01
janhavigupta007 pushed a commit to janhavigupta007/glide-for-redis that referenced this pull request Sep 11, 2024
* Python: Add Script commands

Signed-off-by: TJ Zhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Python wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants