Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support for BZMPOP, BZPOPMIN and BZPOPMAX #240

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

gerzse
Copy link
Contributor

@gerzse gerzse commented Jan 25, 2024

Add support for the BZMPOP, BZPOPMIN, BZPOPMAX commands.

Issues #232, #233 and #234.

@gerzse gerzse requested review from chayim and shacharPash January 25, 2024 09:52
@gerzse gerzse self-assigned this Jan 25, 2024
@gerzse gerzse changed the title Draft: Add support for BZMPOP Add support for BZMPOP Jan 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (980a77c) 94.16% compared to head (8948d82) 94.12%.

Files Patch % Lines
src/NRedisStack/CoreCommands/CoreCommandBuilder.cs 82.35% 4 Missing and 2 partials ⚠️
...edisStack/CoreCommands/DataTypes/MinMaxModifier.cs 66.66% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   94.16%   94.12%   -0.04%     
==========================================
  Files          85       87       +2     
  Lines        5178     5279     +101     
  Branches      487      496       +9     
==========================================
+ Hits         4876     4969      +93     
- Misses        178      183       +5     
- Partials      124      127       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gerzse gerzse force-pushed the add-support-for-bzmpop branch 3 times, most recently from d075e7c to 579d1d8 Compare January 29, 2024 18:22
Copy link
Contributor

@shacharPash shacharPash left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gerzse gerzse force-pushed the add-support-for-bzmpop branch 4 times, most recently from c7ae0f5 to 51b5feb Compare January 30, 2024 16:06
Fix some typos in the markdown files and run `dotnet format` on existing
code.
Change the settings so that documentation is included in the build.
@gerzse gerzse force-pushed the add-support-for-bzmpop branch 4 times, most recently from 4aafc50 to 5870af6 Compare February 6, 2024 11:20
@gerzse gerzse changed the title Add support for BZMPOP Add support for BZMPOP, BZPOPMIN and BZPOPMAX Feb 6, 2024
@gerzse gerzse force-pushed the add-support-for-bzmpop branch from 5870af6 to 410c474 Compare February 6, 2024 11:21
Add support for the BZMPOP, BZPOPMIN, BZPOPMAX commands.

Issues redis#232, redis#233 and redis#234.

These commands are blocking on the server, so they go against the current
policy of the StackExchange.Redis library. Therefore make it obvious in
the code documentation that attention must be given to the timeout in
the connection multiplexer, client-side.

The StackExchange.Redis library already defines a type for the payload
returned by BZMPOP (which is the same as for ZMPOP), namely the
SortedSetPopResult class. However, the constructor of that class is
internal in the library, so we can't create instances of it. Therefore
roll our out type for a <value, score> pair, and use Tuple to pair a key
with a list of such <value, score> pairs.

Instead of using Order to signal from which end of the sorted set to
pop, define a MinMaxModifier enum, which more clearly expresses the
intention and maps directly to the Redis command being executed.
@gerzse gerzse force-pushed the add-support-for-bzmpop branch from 410c474 to 8948d82 Compare February 6, 2024 11:37
Copy link
Contributor

@shacharPash shacharPash left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@shacharPash shacharPash merged commit fe18a3b into redis:master Feb 13, 2024
16 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants