Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

[WIP] Use asyncio protocol #1287

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

seandstewart
Copy link
Collaborator

This supersedes #1216 and is an attempt to address the latency issues in #1208 -

Mainly I've gone through and added a Python parser which matches the hiredis implementation as closely as possible. I've also made a few adjustments to the RedisProtocol as introduced by @m-novikov in #1216.

I spent all day yesterday trying to get perfomance for this port to match something close to what @artesby & @m-novikov reported in their benchmarks in #1208, but I have a suspicion it's not possible.

@m-novikov and @artesby I'd be much obliged if you could test this implementation to see if we're actually going the right direction with this change. It's not yet ready for the real world and there are a few failing tests, but I don't want to keep barking up this tree if it won't get us where we need to go.

@lgtm-com
Copy link

lgtm-com bot commented Feb 6, 2022

This pull request introduces 6 alerts and fixes 1 when merging fbedfbe into 659a14d - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for Suspicious unused loop iteration variable
  • 1 for Wrong name for an argument in a class instantiation

fixed alerts:

  • 1 for Missing call to `__init__` during object initialization

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #1287 (659a14d) into master (659a14d) will not change coverage.
The diff coverage is n/a.

❗ Current head 659a14d differs from pull request most recent head 834d431. Consider uploading reports for the commit 834d431 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1287   +/-   ##
=======================================
  Coverage   90.75%   90.75%           
=======================================
  Files          21       21           
  Lines        6947     6947           
  Branches      889      889           
=======================================
  Hits         6305     6305           
  Misses        469      469           
  Partials      173      173           
Flag Coverage Δ
unit 90.68% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 659a14d...834d431. Read the comment docs.

Copy link
Collaborator

@bmerry bmerry left a comment

Choose a reason for hiding this comment

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

Sorry, I'm now in a different team that's not working with aioredis, so I'm not available for reviewing.

@Andrew-Chen-Wang
Copy link
Collaborator

thanks for all the help along the way @bmerry !

@seandstewart
Copy link
Collaborator Author

General update here -

Anything involving a MONITOR or PubSub SUBSCRIBE is not currently compatible. This is because both of these commands create an open channel which streams data as it is received, but the current Protocol implementation attaches all received data into a single future which is associated to the original packed command. We'll need an alternative implementation which allows these commands to listen on the stream decoupled from the original sent command.

@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2022

This pull request introduces 2 alerts and fixes 1 when merging 834d431 into 659a14d - view on LGTM.com

new alerts:

  • 1 for Redundant assignment
  • 1 for Wrong name for an argument in a class instantiation

fixed alerts:

  • 1 for Missing call to `__init__` during object initialization

@seandstewart seandstewart mentioned this pull request Feb 9, 2022
@matemax
Copy link

matemax commented May 30, 2022

Hi guys. Any update? Performance degradation is blocks an update to 2.* version in our projects =(

@Andrew-Chen-Wang
Copy link
Collaborator

@matemax all updates are going to redis/redis-py. This repo at some point will convert back to its 1.3.1 version

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

Successfully merging this pull request may close these issues.

None yet

5 participants