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

Fix health check after unsubscribe#1207

Open
Andrew-Chen-Wang wants to merge 10 commits intomasterfrom
health-check-unsubscribe-fix
Open

Fix health check after unsubscribe#1207
Andrew-Chen-Wang wants to merge 10 commits intomasterfrom
health-check-unsubscribe-fix

Conversation

@Andrew-Chen-Wang
Copy link
Copy Markdown
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang commented Nov 17, 2021 •
edited
Loading

What do these changes do?

Returns None for health check response.

Are there changes in behavior for the user?

Maybe. If someone used to check the health check response after unsubscribing (for some odd reason), then yes. But for most people, no. And I can't imagine why since we usually return None for health checks.

Related issue number

Fixes #1206

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES/ folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example:
      Fix issue with non-ascii contents in doctest text files.

@Andrew-Chen-Wang Andrew-Chen-Wang mentioned this pull request Nov 17, 2021
Open
1 task
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 17, 2021 •
edited
Loading

Codecov Report

Merging #1207 (5e0ceea) into master (a708bd1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master    #1207   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files          21       21           
  Lines        6872     6873    +1     
  Branches      794      794           
=======================================
+ Hits         6131     6132    +1     
  Misses        578      578           
  Partials      163      163
Flag Coverage Δ
unit 89.13% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
aioredis/client.py 82.56% <100.00%> (+<0.01%) ⬆️

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 a708bd1...d4fe77c. Read the comment docs.

@bmerry
Copy link
Copy Markdown
Collaborator

bmerry commented Nov 22, 2021

The code LGTM for this specific issue, but it is probably worth adding a comment explaining why you're checking for two possible responses.

bmerry
bmerry suggested changes Nov 22, 2021
View reviewed changes
Comment thread aioredis/client.py Outdated
@bmerry bmerry mentioned this pull request Nov 22, 2021
Open
1 task
@Andrew-Chen-Wang @bmerry
Update aioredis/client.py
5e0ceea 
Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>
@bmerry bmerry mentioned this pull request Nov 23, 2021
Closed
@Andrew-Chen-Wang
Copy link
Copy Markdown
Collaborator Author

Andrew-Chen-Wang commented Dec 24, 2021

not sure what's going on. Will come back later if no one else figures out what's wrong. Branch should be open for anyone to change...

@Andrew-Chen-Wang Andrew-Chen-Wang mentioned this pull request Dec 28, 2021
Open
8 tasks
@Andrew-Chen-Wang Andrew-Chen-Wang linked an issue Dec 28, 2021 that may be closed by this pull request
Pubsub: health check races with get_message to read from the socket #1217
Open
1 task
@Andrew-Chen-Wang Andrew-Chen-Wang mentioned this pull request Feb 12, 2022
Merged
4 tasks
@Andrew-Chen-Wang Andrew-Chen-Wang mentioned this pull request Apr 19, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@seandstewart seandstewart Awaiting requested review from seandstewart

1 more reviewer

@bmerry bmerry bmerry approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

bot:chronographer:provided bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pubsub: health check races with get_message to read from the socket Health check fails when a pubsub has no subscriptions

3 participants

@Andrew-Chen-Wang @bmerry @seandstewart