Python: add BZPOPMIN and BZPOPMAX commands #1399

aaron-congo · 2024-05-09T23:08:41Z

Issue #, if available:
N/A

Description of changes:

redis docs:
- https://redis.io/docs/latest/commands/bzpopmin/
- https://redis.io/docs/latest/commands/bzpopmax/
includes value conversion:
- BZPOPMIN/BZPOPMAX returns an array containing a key, member, and score. RESP2 returns the score as a string, but RESP3 returns the score as a double. The value conversion added in this PR ensures the score is always returned as a double

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

GumpacG · 2024-05-10T19:01:58Z

glide-core/src/client/value_conversion.rs

@@ -21,6 +21,7 @@ pub(crate) enum ExpectedReturnType {
    Lolwut,
    ArrayOfArraysOfDoubleOrNull,
    ArrayOfKeyValuePairs,
+    KeyWithMemberAndScore,


How come this wasn't included in the Java implementation?

ig it was forgotten, good catch:)

Yeah its because the conversion is needed for RESP2 responses. The Java wrapper doesn't support RESP2 yet but Python does

GumpacG · 2024-05-10T20:36:14Z

glide-core/src/client/value_conversion.rs

+                if array.len() == 3
+                    && matches!(array[2], Value::BulkString(_) | Value::SimpleString(_)) =>
+            {
+                array[2] =


Does this only convert the third argument to double? Can there be more arguments with more keys where the timeout is greater than the third argument?

redis> ZADD zset1 0 a 1 b 2 c (integer) 3 redis> BZPOPMIN zset1 zset2 0 1) "zset1" 2) "a" 3) "0"

replay can be one of the following:

Nil reply: when no element could be popped and the timeout expired.
Array reply: the keyname, popped member, and its score.

so when its an array replay, which means we popped an element, we want to convert just the score from string to double

Gotcha, thanks!

shohamazon · 2024-05-13T12:17:05Z

glide-core/src/client/value_conversion.rs

@@ -21,6 +21,7 @@ pub(crate) enum ExpectedReturnType {
    Lolwut,
    ArrayOfArraysOfDoubleOrNull,
    ArrayOfKeyValuePairs,
+    KeyWithMemberAndScore,


ig it was forgotten, good catch:)

shohamazon · 2024-05-13T12:19:57Z

glide-core/src/client/value_conversion.rs

+                if array.len() == 3
+                    && matches!(array[2], Value::BulkString(_) | Value::SimpleString(_)) =>
+            {
+                array[2] =


redis> ZADD zset1 0 a 1 b 2 c (integer) 3 redis> BZPOPMIN zset1 zset2 0 1) "zset1" 2) "a" 3) "0"

replay can be one of the following:

Nil reply: when no element could be popped and the timeout expired.
Array reply: the keyname, popped member, and its score.

so when its an array replay, which means we popped an element, we want to convert just the score from string to double

shohamazon · 2024-05-13T12:22:53Z

python/python/glide/async_commands/core.py

+        BZPOPMAX is the blocking variant of ZPOPMAX.
+
+        BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.


Suggested change

BZPOPMAX is the blocking variant of ZPOPMAX.

BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

`BZPOPMAX` is the blocking variant of `ZPOPMAX`.

`BZPOPMAX` is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

maybe we should do:

BBZPOPMAX is a client blocking command, see [Blocking commands](https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands) for more details and best practices.

I don't think hyperlinks like these are supported by default, according to this stack overflow post they aren't, but some tools and/or documentation generators may support them. Looks like VScode does, but I'm using pycharm and it doesn't. Let me know if you still want me to change this to a hyperlink though

python/python/glide/async_commands/core.py

python/python/glide/async_commands/transaction.py

python/python/tests/test_async_client.py

aaron-congo · 2024-05-13T21:49:51Z

@shohamazon Thanks for the review, all comments are addressed now

Yury-Fridlyand · 2024-05-13T23:48:27Z

@aaron-congo please, resolve conflicts and merge

* Python: add BZPOPMIN and BZPOPMAX commands (#266) * Update PR link * PR suggestions * Fix rust

aaron-congo requested a review from a team as a code owner May 9, 2024 23:08

aaron-congo added the python label May 9, 2024

aaron-congo requested review from Yury-Fridlyand, acarbonetto, GumpacG, jonathanl-bq, tjzhang-BQ, barshaul and shohamazon and removed request for Yury-Fridlyand May 9, 2024 23:09

Yury-Fridlyand approved these changes May 10, 2024

View reviewed changes

GumpacG approved these changes May 10, 2024

View reviewed changes

shohamazon reviewed May 13, 2024

View reviewed changes

aaron-congo force-pushed the python/integ_acongo_bzpopmax branch from a67c7ba to 3f1e3c4 Compare May 13, 2024 21:12

acarbonetto approved these changes May 13, 2024

View reviewed changes

Elen-Ghulam approved these changes May 13, 2024

View reviewed changes

aaron-congo and others added 4 commits May 13, 2024 16:54

Python: add BZPOPMIN and BZPOPMAX commands (#266)

63256d3

Update PR link

93bc0fd

PR suggestions

abefd9d

Fix rust

6a23610

aaron-congo force-pushed the python/integ_acongo_bzpopmax branch from 9c3e15f to 6a23610 Compare May 13, 2024 23:54

acarbonetto merged commit 8c590a7 into aws:main May 14, 2024
45 checks passed

acarbonetto deleted the python/integ_acongo_bzpopmax branch May 14, 2024 05:25

avifenesh pushed a commit that referenced this pull request May 14, 2024

Python: add BZPOPMIN and BZPOPMAX commands (#1399)

5deb969

* Python: add BZPOPMIN and BZPOPMAX commands (#266) * Update PR link * PR suggestions * Fix rust

shohamazon mentioned this pull request May 15, 2024

Python: add BZMPOP command #1412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: add BZPOPMIN and BZPOPMAX commands #1399

Python: add BZPOPMIN and BZPOPMAX commands #1399

aaron-congo commented May 9, 2024

GumpacG May 10, 2024

shohamazon May 13, 2024

aaron-congo May 13, 2024

GumpacG May 10, 2024

shohamazon May 13, 2024

GumpacG May 13, 2024

shohamazon May 13, 2024

shohamazon May 13, 2024

shohamazon May 13, 2024

shohamazon May 13, 2024

shohamazon May 13, 2024

aaron-congo May 13, 2024 •

edited

aaron-congo commented May 13, 2024

Yury-Fridlyand commented May 13, 2024

		BZPOPMAX is the blocking variant of ZPOPMAX.

		BZPOPMAX is a client blocking command, see https://github.com/aws/glide-for-redis/wiki/General-Concepts#blocking-commands for more details and best practices.

Python: add BZPOPMIN and BZPOPMAX commands #1399

Python: add BZPOPMIN and BZPOPMAX commands #1399

Conversation

aaron-congo commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-congo May 13, 2024 • edited

Choose a reason for hiding this comment

aaron-congo commented May 13, 2024

Yury-Fridlyand commented May 13, 2024

aaron-congo May 13, 2024 •

edited