redis-py icon indicating copy to clipboard operation
redis-py copied to clipboard

Execute TimeSeries commands not including keys in their syntax on the default node of a Redis Cluster

Open Ernest0x opened this issue 3 years ago • 4 comments

Some commands of the TimeSeries module, such as TS.MGET, TS.MRANGE, TS.MREVRANGE and TS.QUERYINDEX, do not include explicitly the keys (time series) they will operate on in their syntax. Instead, any keys will be possibly matched through filters (by label, value, etc.) that are part of the syntax. Moreover, the matched keys could be stored in different slots of different cluster nodes (shards). So, when we use RedisCluster client to execute these commands on a Redis Cluster, it makes no sense to use the client's slot finding mechanism in order to choose the node to execute the command (which is the default mechanism for most other commands), because there is no key to be used for that in the arguments of the command. With this change, the aforementioned commands will be handled separately and sent to the default cluster node instead.

A fix is also included for a case in which the server may respond on the "COMMAND" command with a "subcommands" field with an empty list as its value, which, along with some other attributes in the response, would lead to an exception because the "subcommands" field was not checked before for equaling to an empty list.

Ernest0x avatar Nov 22 '22 18:11 Ernest0x

Codecov Report

Patch coverage: 42.85% and project coverage change: -0.02 :warning:

Comparison is base (8bfd492) 92.29% compared to head (1224ab1) 92.27%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
- Coverage   92.29%   92.27%   -0.02%     
==========================================
  Files         115      115              
  Lines       29759    29763       +4     
==========================================
  Hits        27465    27465              
- Misses       2294     2298       +4     
Impacted Files Coverage Δ
redis/commands/parser.py 62.63% <0.00%> (-0.70%) :arrow_down:
redis/cluster.py 90.34% <75.00%> (-0.09%) :arrow_down:

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Nov 22 '22 18:11 codecov-commenter

Please, can someone review this? I think it is straightforward.

I should also mention, in case this has not been clear, that this is actually a bug fix. Without this patch, the TimeSeries commands mentioned above cause an exception when executed on Redis Cluster. I have no permission to add the "bug" issue label myself though.

Ernest0x avatar Dec 06 '22 09:12 Ernest0x

@dvora-h If you have a couple of minutes, I would appreciate it if you could review this.

Ernest0x avatar Jan 18 '23 20:01 Ernest0x

This pull request is marked stale. It will be closed in 30 days if it is not updated.

github-actions[bot] avatar Mar 16 '24 00:03 github-actions[bot]