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

i40: Support for Redis 6.2.0 #41

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft

i40: Support for Redis 6.2.0 #41

wants to merge 20 commits into from

Conversation

richfitz
Copy link
Owner

@richfitz richfitz commented Nov 2, 2020

Adds support for new commands added through to Redis 6.2.0, incorporating changes in #28, fixing failing tests and adding tests for the new functions.

The interface to the stream (X*) functions I am not immediately happy with, as things like XREADGROUP I am not sure are properly supported yet, and commands like XGROUP really feel like they should be split into subcommands. It might be easiest to fold this in for now and then fix, but to do this properly I probably will have to either understand the streaming interface or get input from someone who has used it (@dseynaev perhaps?).

Fixes #40

@codecov-io
Copy link

codecov-io commented Nov 2, 2020

Codecov Report

Merging #41 (8ba428e) into master (83b0614) will decrease coverage by 0.60%.
The diff coverage is 95.41%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #41      +/-   ##
===========================================
- Coverage   100.00%   99.39%   -0.61%     
===========================================
  Files           18       18              
  Lines         1476     1642     +166     
===========================================
+ Hits          1476     1632     +156     
- Misses           0       10      +10     
Impacted Files Coverage Δ
R/redis_api_generated.R 98.87% <95.39%> (-1.13%) ⬇️
R/util_assert.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83b0614...8ba428e. Read the comment docs.

@r2evans
Copy link

r2evans commented Dec 1, 2020

The interface to the stream (X*) functions I am not immediately happy with, as things like XREADGROUP I am not sure are properly supported yet, and commands like XGROUP really feel like they should be split into subcommands.

IIUC, are you suggesting that there might be a set of subcommands not normally available in r <- hiredis() that would allow generation of a consumer group and only then provide relevant subfunctions?

r <- hiredis()
mygrp <- r$consumer_group("mystream", "mygroup", "myname")
mygrp$XADD(...)
mygrp$XREAD(...)

(Perhaps mangled a bit.) I don't like that interface, to be honest.

I think the most straight-forward path for redux would be to provide a "simple" flat API, supporting the X* functions exactly the same as the others. The fact that some of them accept arbitrary numbers of arguments (e.g., topics and offsets) is something that can be managed with a little bit of work. If there is any encapsulation of these basic functions into a more OO architecture, I argue it can be done on top of the vanilla functions.

My vote: add XADD and friends as-is. If there are issues with order of arguments (optional), then I don't see them yet, and don't know what errors I'm making in this recommendation.

@kevinykuo
Copy link

+1 that the streaming features should be exposed at the level consistent with existing functions, and sugar can be built on top of them later, either in this package (with a different naming scheme ideally) or elsewhere.

@richfitz
Copy link
Owner Author

Thanks @kevinykuo and @r2evans for the feedback. I will pick this back up as soon as I have time again for personal open source work (I've been knee-deep in covid work since mid-November with no spare time)

@codecov-commenter
Copy link

Codecov Report

Merging #41 (29ee6f6) into master (1756f97) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 29ee6f6 differs from pull request most recent head 8251240. Consider uploading reports for the commit 8251240 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            master       #41    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           18        18            
  Lines         1479      1722   +243     
==========================================
+ Hits          1479      1722   +243     
Impacted Files Coverage Δ
src/conversions.c 100.00% <ø> (ø)
R/redis_api_generated.R 100.00% <100.00%> (ø)
R/util_assert.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1756f97...8251240. Read the comment docs.

@richfitz
Copy link
Owner Author

Support up to ~6.0.0 is ok, but after that we get a new block argument type that I need to handle:

See redis/redis-doc#1443 mmkal/handy-redis#255

@r2evans
Copy link

r2evans commented Jan 13, 2023

@richfitz, I recognize that redux works well enough for your needs, and you haven't had time to prioritize towards it in the last couple of years. Have you considered allowing anyone else access to accept PRs? I admittedly don't have a lot of bandwidth, but perhaps I can provide just enough help to allow some much-needed (for others) updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for redis 6.2.0 commands
6 participants