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.

Sign up for GitHub

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

Jump to bottom

Add support for PubSub with RESP3 parser #2721

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Add support for PubSub with RESP3 parser #2721

merged 5 commits into from
Apr 24, 2023

Conversation

dvora-h
Copy link
Collaborator

@dvora-h dvora-h commented Apr 23, 2023

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

#2648

@dvora-h dvora-h added the feature New feature label Apr 23, 2023
@dvora-h dvora-h requested a review from chayim April 23, 2023 01:42
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 62.60% and project coverage change: -0.13 ⚠️

Comparison is base (753018e) 91.94% compared to head (dff35f9) 91.82%.

📣 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             @@
##              5.0    #2721      +/-   ##
==========================================
- Coverage   91.94%   91.82%   -0.13%     
==========================================
  Files         121      121              
  Lines       30110    30204      +94     
==========================================
+ Hits        27685    27734      +49     
- Misses       2425     2470      +45
Impacted Files Coverage Δ
redis/parsers/resp3.py 51.58% <44.44%> (-5.26%) ⬇️
redis/asyncio/connection.py 84.85% <50.00%> (-0.36%) ⬇️
tests/test_asyncio/test_pubsub.py 98.18% <61.90%> (-1.20%) ⬇️
tests/test_pubsub.py 98.11% <61.90%> (-1.50%) ⬇️
redis/client.py 88.56% <81.81%> (-0.13%) ⬇️
redis/asyncio/client.py 92.44% <83.33%> (-0.39%) ⬇️
redis/connection.py 82.25% <83.33%> (-0.08%) ⬇️
redis/utils.py 91.30% <100.00%> (+1.14%) ⬆️

... and 2 files 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.

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

chayim
chayim reviewed Apr 23, 2023
View reviewed changes
redis/client.py Outdated
@@ -1429,6 +1429,7 @@ def __init__(
shard_hint=None,
ignore_subscribe_messages=False,
encoder=None,
push_handler=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

push_handler_func

and the type hint that goes with it

chayim
chayim reviewed Apr 23, 2023
View reviewed changes
redis/utils.py Outdated
handler.setLevel(logging.INFO)
logger.addHandler(handler)

INFO_LOGGER = logger
Copy link
Contributor

Choose a reason for hiding this comment

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

INFO_LOGGER Should instead be _info_logger, so that we don't get caught in the same issue as the parsers rename

chayim
chayim reviewed Apr 23, 2023
View reviewed changes
redis/utils.py Outdated Show resolved Hide resolved
chayim
chayim suggested changes Apr 23, 2023
View reviewed changes
Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

Small changes - the logger approach

chayim
chayim reviewed Apr 24, 2023
View reviewed changes
redis/asyncio/client.py
@@ -656,6 +656,7 @@ def __init__(
shard_hint: Optional[str] = None,
ignore_subscribe_messages: bool = False,
encoder=None,
push_handler_func: Optional[Callable] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

really happy to see the same option name ❤️

chayim
chayim reviewed Apr 24, 2023
View reviewed changes
redis/utils.py
@@ -117,3 +118,12 @@ def wrapper(*args, **kwargs):
return wrapper

return decorator


def _set_info_logger():
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring or comment on why it exists, please.

@dvora-h dvora-h merged commit a96a38a into redis:5.0 Apr 24, 2023
@dvora-h dvora-h mentioned this pull request Apr 30, 2023
Closed
dvora-h added a commit that referenced this pull request Jul 16, 2023
@dvora-h @chayim
@leibale @bellini666 @woutdenolf @shacharPash @jamestiotio @mirekdlugosz @oranav @mzdehbashi-github @tylerhutcherson @felipou @AYMENJD @scoopex @Avasam @Anthchirp @kristjanvalur @NickG123 @cristianmatache @SoulPancake @aciddust @bodevone @Smit-Parmar @bmacphee @uglide @shahar-lev @vmihailenco @TheKevJames
Merge 5.0 to master (#2849)
2732a85 
* Reorganizing the parsers code, and add support for RESP3 (#2574)

* Reorganizing the parsers code

* fix build package

* fix imports

* fix flake8

* add resp to Connection class

* core commands

* python resp3 parser

* pipeline

* async resp3 parser

* some asymc tests

* resp3 parser for async cluster

* async commands tests

* linters

* linters

* linters

* fix ModuleNotFoundError

* fix tests

* fix assert_resp_response_in

* fix command_getkeys in cluster

* fail-fast false

* version

---------

Co-authored-by: Chayim I. Kirshen <[email protected]>

* Fix async client with resp3 (#2657)

* Add support for PubSub with RESP3 parser (#2721)

* add resp3 pubsub

* linters

* _set_info_logger func

* async pubsun

* docstring

* 5.0.0b2 (#2723)

* Fix `COMMAND` response in resp3 (redis 7+) (#2740)

* Fix protocol version checking (#2737)

* bumping beta version to 5.0.0b3 (#2743)

* Fix parse resp3 dict response: don't use dict comprehension (#2757)

* Fix parse respp3 dict response

* linters

* pin urlib version

* Sharded pubsub (#2762)

* sharded pubsub

* sharded pubsub

Co-authored-by: Leibale Eidelman <[email protected]>

* Shrded Pubsub TestPubSubSubscribeUnsubscribe

* fix TestPubSubSubscribeUnsubscribe

* more tests

* linters

* TestPubSubSubcommands

* fix @leibale comments

* linters

* fix @chayim comments

---------

Co-authored-by: Leibale Eidelman <[email protected]>

* 5.0.0b4 (#2781)

Co-authored-by: dvora-h <[email protected]>

* RESP3 tests (#2780)

* fix command response in resp3

* linters

* acl_log & acl_getuser

* client_info

* test_commands and test_asyncio/test_commands

* fix test_command_parser

* fix asyncio/test_connection/test_invalid_response

* linters

* all the tests

* push handler sharded pubsub

* Use assert_resp_response wherever possible

* fix test_xreadgroup

* fix cluster_zdiffstore and cluster_zinter

* fix review comments

* fix review comments

* linters

* Fixing asyncio import (#2759)

* asyncio import fix

* pinning urllib3 to fix CI (#2748)

* noqa

* fixint linters

* fix (#2799)

* RESP3 response callbacks (#2798)

* start cleaning

* clean sone callbacks

* response callbacks

* revert redismod-url change

* fix async tests

* linters

* async cluster

---------

Co-authored-by: Chayim <[email protected]>

* RESP3 modules support (#2803)

* start cleaning

* clean sone callbacks

* response callbacks

* modules

* tests

* finish sync search tests

* linters

* async modules

* linters

* revert redismod-url change

* RESP3 fix async tests (#2806)

* fix tests

* add stralgo callback in resp2

* add callback to acl list in resp2

* Adding RESP3 tests support (#2793)

* start cleaning

* clean sone callbacks

* first phase

* tox wrap back

* changing cancel format

* syntax

* lint

* docker

* contain the docker

* tox dev reqs

* back to testing

* response callbacks

* protocol into async conftest

* fix for 3.11 invoke

* docker changes

* fix tests

* linters

* adding

* resp3 tox, until killed

* remove tox

* tests

* requirements.txt

* restoring requirements.txt

* adding a sleep, hopefully enough time for the cluster dockers to settle

* fix search tests

* search test, disable uvloop for pypy due to bug

* syn

* reg

* dialect test improvement

* sleep+, xfail

* tests

* resp

* flaky search test too

* timing

* timing for async test

* test changes

* fix assert_interval_advanced

* revert

* mark async health_check tests with xfail

* change strict to false

* fix github actions package validation

---------

Co-authored-by: dvora-h <[email protected]>

* change sismember return type (#2813)

* Version 5.0.0rc1 (#2815)

* Merge master to 5.0 (#2827)

* fix: do not use asyncio's timeout lib before 3.11.2 (#2659)

There's an issue in asyncio's timeout lib before 3.11.3 that causes
async calls to raise `CancelledError`.

This is a cpython issue that was fixed in this commit [1] and
cherry-picked to previous versions, meaning 3.11.3 will work correctly.

Check [2] for more info.

[1] python/cpython@04adf2d
[2] #2633

* UnixDomainSocketConnection missing constructor argument (#2630)

* removing useless files (#2642)

* Fix issue 2660: PytestUnraisableExceptionWarning from asycio client (#2669)

* Fixing cancelled async futures (#2666)

Co-authored-by: James R T <[email protected]>
Co-authored-by: dvora-h <[email protected]>

* Fix async (#2673)

* Version 4.5.4 (#2674)

* Really do not use asyncio's timeout lib before 3.11.2 (#2699)

4802530 made async-timeout required
only on Python 3.11.2 and earlier. However, according to PEP-508,
python_version marker is compared to first two numbers of Python version
tuple - so it will evaluate to True also on 3.11.3, and install a
package as a dependency.

* asyncio: Fix memory leak caused by hiredis (#2693) (#2694)

* Update example of Redisearch creating index (#2703)

When creating index, fields should be passed inside an iterable (e.g. list or tuple)

* Improving Vector Similarity Search Example (#2661)

* update vss docs

* add embeddings creation and storage examples

* update based on feedback

* fix version and link

* include more realistic search examples and clean up indices

* completely remove initial cap reference

---------

Co-authored-by: Chayim <[email protected]>

* Fix incorrect usage of once flag in async Sentinel (#2718)

In the execute_command of the async Sentinel, the once flag was being
used incorrectly, with its meaning inverted. To fix we just needed to invert
the if and else bodies. This isn't being caught by the tests currently
because the tests of commands that use this flag do not check their
results/effects (for example the "test_ckquorum" test).

* Fix topk list example. (#2724)

* Improve error output for master discovery (#2720)

Make MasterNotFoundError exception more precise in the case of
ConnectionError and TimeoutError to help the user to identify
configuration errors

Co-authored-by: Marc Schöchlin <[email protected]>

* return response in case of KeyError (#2628)

* return response in case of KeyError

* fix code linters error

* fix linters 2

* fix linters 3

* Add WITHSCORES to ZREVRANK Command (#2725)

* add withscores to zrevrank

* change 0 -> 2

* fix errors

* split test

* Fix `ClusterCommandProtocol` not itself being marked as a protocol (#2729)

* Fix `ClusterCommandProtocol` not itself being marked as a protocol

* Update CHANGES

* Fix potential race condition during disconnection (#2719)

When the disconnect() function is called twice in parallel it is possible that
one thread deletes the self._sock reference, while the other thread will
attempt to call .close() on it, leading to an AttributeError.

This situation can routinely be encountered by closing the connection in a
PubSubWorkerThread error handler in a blocking thread (ie. with
sleep_time==None), and then calling .close() on the PubSub object.
The main thread will then run into the disconnect() function, and the listener
thread is woken up by the closure and will race into the disconnect()
function, too.

This can be fixed easily by copying the object reference before doing the
None-check, similar to what we do in the redis.client.close() function.

* add "address_remap" feature to RedisCluster (#2726)

* add cluster "host_port_remap" feature for asyncio.RedisCluster

* Add a unittest for asyncio.RedisCluster

* Add host_port_remap to _sync_ RedisCluster

* add synchronous tests

* rename arg to `address_remap` and take and return an address tuple.

* Add class documentation

* Add CHANGES

* nermina changes from NRedisStack (#2736)

* Updated AWS Elasticache IAM Connection Example (#2702)

Co-authored-by: Nick Gerow <[email protected]>

* pinning urllib3 to fix CI (#2748)

* Add RedisCluster.remap_host_port, Update tests for CWE 404 (#2706)

* Use provided redis address. Bind to IPv4

* Add missing "await" and perform the correct test for pipe eimpty

* Wait for a send event, rather than rely on sleep time. Excpect cancel errors.

* set delay to 0 except for operation we want to cancel
This speeds up the unit tests considerably by eliminating unnecessary delay.

* Release resources in test

* Fix cluster test to use address_remap and multiple proxies.

* Use context manager to manage DelayProxy

* Mark failing pipeline tests

* lint

* Use a common "master_host" test fixture

* Update redismodules.rst (#2747)

Co-authored-by: dvora-h <[email protected]>

* Add support for cluster myshardid (#2704)

* feat: adding support for cluster myshardid

* lint fix

* fix: comment fix and async test

* fix: adding version check

* fix lint:

* linters

---------

Co-authored-by: Anuragkillswitch <[email protected]>
Co-authored-by: dvora-h <[email protected]>
Co-authored-by: dvora-h <[email protected]>

* clean warnings (#2731)

* fix parse_slowlog_get (#2732)

* Optionally disable disconnects in read_response (#2695)

* Add regression tests and fixes for issue #1128

* Fix tests for resumable read_response to use "disconnect_on_error"

* undo prevision fix attempts in async client and cluster

* re-enable cluster test

* Suggestions from code review

* Add CHANGES

* Add client no-touch (#2745)

* Add client no-touch

* Update redis/commands/core.py

Co-authored-by: dvora-h <[email protected]>

* Update test_commands.py

Improve test_client_no_touch

* Update test_commands.py

Add async version test case

* Chore remove whitespace

Oops

---------

Co-authored-by: dvora-h <[email protected]>

* fix create single_connection_client from url (#2752)

* Fix `xadd` allow non negative maxlen (#2739)

* Fix xadd allow non negative maxlen

* Update change log

---------

Co-authored-by: dvora-h <[email protected]>

* Version 4.5.5 (#2753)

* Kristjan/issue #2754: Add missing argument to SentinelManagedConnection.read_response() (#2756)

* Increase timeout for a test which would hang completely if failing.
Timeouts in virtualized CI backends can occasionally fail if too short.

* add "disconnect_on_error" argument to SentinelManagedConnection

* update Changes

* lint

* support JSON.MERGE Command (#2761)

* support JSON.MERGE Command

* linters

* try with abc instead person

* change @skip_ifmodversion_lt to latest ReJSON 2.4.7

* change version

* fix test

* linters

* add async test

* Issue #2749: Remove unnecessary __del__ handlers (#2755)

* Remove unnecessary __del__ handlers
There normally should be no logic attached to del.  Cleanly disconnecting network resources is not needed at that time.

* add CHANGES

* Add WITHSCORE to ZRANK (#2758)

* add withscore to zrank with tests

* fix test

* Fix JSON.MERGE Summary (#2786)

* Fix JSON.MERGE Summary

* linters

* Fixed key error in parse_xinfo_stream (#2788)

* insert newline to prevent sphinx from assuming code block (#2796)

* Introduce OutOfMemoryError exception for Redis write command rejections due to OOM errors (#2778)

* expose OutOfMemoryError as explicit exception type

- handle "OOM" error code string by raising explicit
  exception type instance
- enables callers to avoid string matching after
  catching ResponseError

* add OutOfMemoryError exception class docstring

* Provide more info in the exception docstring

* Fix formatting

* Again

* linters

---------

Co-authored-by: Chayim <[email protected]>
Co-authored-by: Igor Malinovskiy <[email protected]>
Co-authored-by: dvora-h <[email protected]>

* Add unit tests for the `connect` method of all Redis connection classes (#2631)

* tests: move certificate discovery to a separate module

* tests: add 'connect' tests for all Redis connection classes

---------

Co-authored-by: dvora-h <[email protected]>

* Fix dead weakref in sentinel connection causing ReferenceError (#2767) (#2771)

* Fix dead weakref in sentinel conn (#2767)

* Update CHANGES

---------

Co-authored-by: Igor Malinovskiy <[email protected]>
Co-authored-by: dvora-h <[email protected]>

* chore(documentation): fix redirects and some small cleanups (#2801)

* Add waitaof (#2760)

* Add waitaof

* Update test_commands.py

add test_waitaof

* Update test_commands.py

Add test_waitaof

* Fix doc string

---------

Co-authored-by: Chayim <[email protected]>
Co-authored-by: Igor Malinovskiy <[email protected]>

* Extract abstract async connection class (#2734)

* make 'socket_timeout' and 'socket_connect_timeout' equivalent for TCP and UDS connections

* abstract asynio connection in analogy with the synchronous connection

---------

Co-authored-by: dvora-h <[email protected]>

* Fix type hint for retry_on_error in async cluster (#2804)

* fix(asyncio.cluster): fixup retry_on_error type hint

This parameter accepts a list of _classes of Exceptions_, not a list of instantiated Exceptions. Fixup the type hint accordingly.

* chore: update changelog

---------

Co-authored-by: dvora-h <[email protected]>

* Fix CI (#2809)

* Support JSON.MSET Command (#2766)

* support JSON.MERGE Command

* linters

* try with abc instead person

* change @skip_ifmodversion_lt to latest ReJSON 2.4.7

* change version

* fix test

* linters

* add async test

* Support JSON.MSET command

* trying to run CI

* linters

* add async test

* reminder do delete the integration changes

* delete the line from integration

* fix the interface

* change docstring

---------

Co-authored-by: Chayim <[email protected]>
Co-authored-by: dvora-h <[email protected]>

* Version 4.6.0 (#2810)

* master changes

* linters

* fix test_cwe_404 cluster test

---------

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
Co-authored-by: woutdenolf <[email protected]>
Co-authored-by: Chayim <[email protected]>
Co-authored-by: shacharPash <[email protected]>
Co-authored-by: James R T <[email protected]>
Co-authored-by: Mirek Długosz <[email protected]>
Co-authored-by: Oran Avraham <[email protected]>
Co-authored-by: mzdehbashi-github <[email protected]>
Co-authored-by: Tyler Hutcherson <[email protected]>
Co-authored-by: Felipe Machado <[email protected]>
Co-authored-by: AYMEN Mohammed <[email protected]>
Co-authored-by: Marc Schöchlin <[email protected]>
Co-authored-by: Marc Schöchlin <[email protected]>
Co-authored-by: Avasam <[email protected]>
Co-authored-by: Markus Gerstel <[email protected]>
Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Nick Gerow <[email protected]>
Co-authored-by: Nick Gerow <[email protected]>
Co-authored-by: Cristian Matache <[email protected]>
Co-authored-by: Anurag Bandyopadhyay <[email protected]>
Co-authored-by: Anuragkillswitch <[email protected]>
Co-authored-by: Seongchuel Ahn <[email protected]>
Co-authored-by: Alibi <[email protected]>
Co-authored-by: Smit Parmar <[email protected]>
Co-authored-by: Brad MacPhee <[email protected]>
Co-authored-by: Igor Malinovskiy <[email protected]>
Co-authored-by: Shahar Lev <[email protected]>
Co-authored-by: Vladimir Mihailenco <[email protected]>
Co-authored-by: Kevin James <[email protected]>

* RESP3 response-callbacks cleanup (#2841)

* cluenup

* sentinel callbacks

* move callbacks

* fix async cluster tests

* _parsers and import fix in tests

* linters

* make modules callbacks private

* fix async search

* fix

---------

Co-authored-by: Chayim I. Kirshen <[email protected]>

* Version 5.0.0rc2 (#2843)

* linters

---------

Co-authored-by: Chayim I. Kirshen <[email protected]>
Co-authored-by: Chayim <[email protected]>
Co-authored-by: Leibale Eidelman <[email protected]>
Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
Co-authored-by: woutdenolf <[email protected]>
Co-authored-by: shacharPash <[email protected]>
Co-authored-by: James R T <[email protected]>
Co-authored-by: Mirek Długosz <[email protected]>
Co-authored-by: Oran Avraham <[email protected]>
Co-authored-by: mzdehbashi-github <[email protected]>
Co-authored-by: Tyler Hutcherson <[email protected]>
Co-authored-by: Felipe Machado <[email protected]>
Co-authored-by: AYMEN Mohammed <[email protected]>
Co-authored-by: Marc Schöchlin <[email protected]>
Co-authored-by: Marc Schöchlin <[email protected]>
Co-authored-by: Avasam <[email protected]>
Co-authored-by: Markus Gerstel <[email protected]>
Co-authored-by: Kristján Valur Jónsson <[email protected]>
Co-authored-by: Nick Gerow <[email protected]>
Co-authored-by: Nick Gerow <[email protected]>
Co-authored-by: Cristian Matache <[email protected]>
Co-authored-by: Anurag Bandyopadhyay <[email protected]>
Co-authored-by: Anuragkillswitch <[email protected]>
Co-authored-by: Seongchuel Ahn <[email protected]>
Co-authored-by: Alibi <[email protected]>
Co-authored-by: Smit Parmar <[email protected]>
Co-authored-by: Brad MacPhee <[email protected]>
Co-authored-by: Igor Malinovskiy <[email protected]>
Co-authored-by: Shahar Lev <[email protected]>
Co-authored-by: Vladimir Mihailenco <[email protected]>
Co-authored-by: Kevin James <[email protected]>
@dvora-h dvora-h mentioned this pull request Jul 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chayim chayim chayim approved these changes

Assignees
No one assigned
Labels
feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dvora-h @codecov-commenter @chayim