Skip to content

Python: Add Batch support #3555

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

BoazBD
Copy link
Collaborator

@BoazBD BoazBD commented Apr 7, 2025

Issue link

This Pull Request is linked to issue (URL): 3546

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@BoazBD BoazBD requested a review from a team as a code owner April 7, 2025 07:26
@BoazBD BoazBD requested a review from Copilot April 7, 2025 07:26
Copilot

This comment was marked as off-topic.

@BoazBD BoazBD force-pushed the boaz/pipelines-pythin-python-wrapper branch from d4fb032 to 41dc46b Compare April 7, 2025 07:35
@Yury-Fridlyand Yury-Fridlyand added python Python wrapper breaking breaking changes labels Apr 7, 2025
@Yury-Fridlyand
Copy link
Collaborator

Please don't forget changelog

@BoazBD BoazBD force-pushed the boaz/pipelines-pythin-python-wrapper branch 6 times, most recently from bae1fb6 to 76dda70 Compare April 10, 2025 14:27
@BoazBD BoazBD requested a review from shohamazon April 15, 2025 13:52
@BoazBD BoazBD changed the title Add python pipeline support Python: Add Batch support Apr 20, 2025
Copy link
Collaborator

@shohamazon shohamazon left a comment

Choose a reason for hiding this comment

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

Great job 🚀

CHANGELOG.md Outdated
@@ -1,5 +1,5 @@
#### Changes

* Python: Add python pipeline support ([#3555](https://github.com/valkey-io/valkey-glide/pull/3555))
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
* Python: Add python pipeline support ([#3555](https://github.com/valkey-io/valkey-glide/pull/3555))
* Python: Add Batches support ([#3555](https://github.com/valkey-io/valkey-glide/pull/3555))

sphinx-rtd-theme
deprecated >= 1.2.18
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
deprecated >= 1.2.18
deprecated >= 1.2.0

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
deprecated >= 1.2.18
deprecated

maybe, u should check whats best

>>> transaction = BaseTransaction()
Transaction vs Pipeline:
- Transactions (is_atomic=True) ensure that all commands are executed atomically.
In a transaction, all keys must belong to the same slot. This means if any key is in a different slot,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In a transaction, all keys must belong to the same slot. - this is not the place for this doc, it should be in ClusterBatch

Copy link
Collaborator

Choose a reason for hiding this comment

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

all this section can be removed


for _, info in result[0].items():
assert isinstance(info, bytes)
assert config_key in info.decode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

confiig_key: config_value in info.decode()


pipeline.get(key)

with pytest.raises(RequestError):
Copy link
Collaborator

Choose a reason for hiding this comment

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

check the error message

assert result[4] == b"value1"
assert result[5] == b"value2"

assert isinstance(result[2], Exception)
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
assert isinstance(result[2], Exception)
assert isinstance(result[2], RequestError)

assert isinstance(result[2], Exception)
assert "wrong number of arguments" in str(result[2])

assert isinstance(result[3], Exception)
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
assert isinstance(result[3], Exception)
assert isinstance(result[3], RequestError)


@pytest.mark.parametrize("cluster_mode", [True, False])
@pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
async def test_pipeline_timeout(self, glide_client: TGlideClient):
Copy link
Collaborator

Choose a reason for hiding this comment

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

merge this with the transaction test and call it test batch timeout

@@ -253,6 +253,7 @@ jobs:
run: |
python -m pip install --upgrade pip
pip install sphinx sphinx-rtd-theme
pip install -r dev_requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note changes coming from #3541

]
)
transaction.fcall(func_name, [], arguments=["one", "two"])
if batch.is_atomic:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? Are there any restrictions for function commands in pipelines?

Copy link
Collaborator

Choose a reason for hiding this comment

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

its because function commands are routed to all nodes, so the response is different

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Apr 23, 2025
20 tasks
@BoazBD BoazBD force-pushed the boaz/pipelines-pythin-python-wrapper branch from 0fe1f67 to d898b8e Compare April 28, 2025 14:11
BoazBD added 2 commits April 28, 2025 14:17
Signed-off-by: BoazBD <[email protected]>
Signed-off-by: BoazBD <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking breaking changes python Python wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants