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: command WAIT #1710

Merged
merged 7 commits into from
Jun 30, 2024
Merged

Conversation

tjzhang-BQ
Copy link
Collaborator

  • Python: command WAIT

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@tjzhang-BQ tjzhang-BQ requested a review from a team as a code owner June 29, 2024 02:57
@tjzhang-BQ tjzhang-BQ force-pushed the python/integ_tjz_wait branch 2 times, most recently from 307c4df to cea8126 Compare June 29, 2024 03:03
@Yury-Fridlyand Yury-Fridlyand added the python Python wrapper label Jun 29, 2024
@Yury-Fridlyand
Copy link
Collaborator

Please apply my comments from #1707 here too

@tjzhang-BQ tjzhang-BQ force-pushed the python/integ_tjz_wait branch from 9cd5a54 to 2701cce Compare June 30, 2024 20:42
CHANGELOG.md Outdated Show resolved Hide resolved
python/python/glide/async_commands/cluster_commands.py Outdated Show resolved Hide resolved
python/python/tests/test_async_client.py Outdated Show resolved Hide resolved
# ensure that command doesn't time out even if timeout > request timeout (250ms by default)
assert await redis_client.set(key, value2) == OK
assert await redis_client.wait(100, 500) >= 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add the following tests that also exist on the Java side?

  • wrap a WAIT call with numreplicas=100 and timeout=0 in a task, ensure it raises a TimeoutError
  • pass negative timeout value, ensure an error occurs

Copy link
Collaborator Author

@tjzhang-BQ tjzhang-BQ Jun 30, 2024

Choose a reason for hiding this comment

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

added bad input test, the other test mainly checks that the command wouldnt time itself out even if the timeout is above the client request timeout, to reflect this setting from glide core, which getting a result from that line should suffice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I guess it also tests the no-timeout functionality but maybe that's not necessary really

Comment on lines +7273 to +7275
assert await redis_client.wait(1, 1000) >= 1
else:
assert await redis_client.wait(1, 1000) >= 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to use == instead of >= since that's what we have in Java?

Suggested change
assert await redis_client.wait(1, 1000) >= 1
else:
assert await redis_client.wait(1, 1000) >= 0
assert await redis_client.wait(1, 1000) == 1
else:
assert await redis_client.wait(1, 1000) == 0

# ensure that command doesn't time out even if timeout > request timeout (250ms by default)
assert await redis_client.set(key, value2) == OK
assert await redis_client.wait(100, 500) >= 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I guess it also tests the no-timeout functionality but maybe that's not necessary really

@acarbonetto acarbonetto merged commit e7cbabb into valkey-io:main Jun 30, 2024
8 checks passed
@acarbonetto acarbonetto deleted the python/integ_tjz_wait branch June 30, 2024 22:47
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jul 16, 2024
* Python: command WAIT

* Python: command WAIT

* changelog

* doc & test changes

* linter

* doc update

* update example

* addressing commends rd2

---------

Co-authored-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
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants