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

Support PING in pubsub mode #264

Merged
merged 3 commits into from
Sep 26, 2017
Merged

Conversation

thomasst
Copy link
Contributor

@thomasst thomasst commented Jul 6, 2017

Fixes #249

@@ -39,6 +39,7 @@
'PSUBSCRIBE', b'PSUBSCRIBE',
'UNSUBSCRIBE', b'UNSUBSCRIBE',
'PUNSUBSCRIBE', b'PUNSUBSCRIBE',
'PING', b'PING',
Copy link
Contributor

Choose a reason for hiding this comment

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

Try removing ping from _PUBSUB_COMMANDS, that is the reason why pool tests are failing

Copy link
Contributor Author

@thomasst thomasst Jul 17, 2017

Choose a reason for hiding this comment

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

This will fail then though:

        is_pubsub = command in _PUBSUB_COMMANDS
        if self._in_pubsub and not is_pubsub:
>           raise RedisError("Connection in SUBSCRIBE mode")
E           aioredis.errors.RedisError: Connection in SUBSCRIBE mode

aioredis/connection.py:274: RedisError

Any other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it will fail.
You can extend condition of that if to something like:

if self._in_pubsub and not (is_pubsub or is_ping):
    ...

@codecov
Copy link

codecov bot commented Jul 17, 2017

Codecov Report

Merging #264 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #264      +/-   ##
=========================================
+ Coverage   96.88%   96.9%   +0.01%     
=========================================
  Files          57      57              
  Lines        7450    7463      +13     
  Branches      587     589       +2     
=========================================
+ Hits         7218    7232      +14     
+ Misses        172     171       -1     
  Partials       60      60
Impacted Files Coverage Δ
tests/pubsub_commands_test.py 100% <100%> (ø) ⬆️
examples/py34/connection.py 93.01% <0%> (-0.17%) ⬇️
tests/sentinel_failover_test.py 88.72% <0%> (+0.75%) ⬆️
aioredis/locks.py 100% <0%> (+4.34%) ⬆️

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 b5604b7...282e880. Read the comment docs.

@thomasst
Copy link
Contributor Author

Just pushed some fixes. I also wanted to get your opinion on the return value of ping(). Normally it returns the string "PONG" (which Redis returns). However, in pubsub mode is returns a (lower case) "pong" message, so for consistency reasons I chose to return the "PONG" string. We might also want to consider returning nothing or True (the regular Python Redis library returns True). What do you think?

@thomasst
Copy link
Contributor Author

@popravich could you please take another look at this PR? Thanks!

@popravich popravich merged commit 6d62869 into aio-libs-abandoned:master Sep 26, 2017
@popravich
Copy link
Contributor

Thanks,
Sorry for long delay

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.

2 participants