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.

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

Extend RedisSettings to include redis Retry Helper settings #387

Merged
merged 14 commits into from
Apr 1, 2024

Conversation

mernmic
Copy link
Contributor

@mernmic mernmic commented Jan 17, 2023

proposed fix for #386

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #387 (e54eace) into main (94cd878) will increase coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head e54eace differs from pull request most recent head a1ac23a. Consider uploading reports for the commit a1ac23a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   96.27%   96.38%   +0.10%     
==========================================
  Files          11       11              
  Lines        1074     1078       +4     
  Branches      209      209              
==========================================
+ Hits         1034     1039       +5     
  Misses         19       19              
+ Partials       21       20       -1     
Files Coverage Δ
arq/connections.py 90.32% <100.00%> (+0.25%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8321dc1...a1ac23a. Read the comment docs.

arq/connections.py Outdated Show resolved Hide resolved
@JonasKs
Copy link
Collaborator

JonasKs commented Jan 18, 2023

The tests has been fixed in #382, so don’t worry about those failing pipelines on some Python versions for now.

As you can see from the codecov bot above, this repo strive to have 100% codecov. Would love if you added tests for these settings.
If you’re unsure how to proceed, please don’t hesitate to ask. 😊

@mernmic
Copy link
Contributor Author

mernmic commented Jan 19, 2023

@JonasKs, added some tests which improved coverage. Still one partial, but might be from testing failures?

@mernmic mernmic requested a review from JonasKs January 19, 2023 01:44
@JonasKs
Copy link
Collaborator

JonasKs commented Jan 20, 2023

I'll try to review again this weekend / early next week 🙇‍♂️

Copy link
Collaborator

@JonasKs JonasKs left a comment

Choose a reason for hiding this comment

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

Looks good to me! 😊

arq/connections.py Outdated Show resolved Hide resolved
@mernmic
Copy link
Contributor Author

mernmic commented Jan 25, 2023

Looks good to me! 😊

Thanks for taking a look, just pushed a change for the Exception type.

We've been using the above changes in our systems and everything has been working as expected so far.

A heads up I'll be offline from the first few weeks of February, so if anything comes up that's the reason for any radio silence on my end.

Thanks again!

@JonasKs
Copy link
Collaborator

JonasKs commented Jan 25, 2023

Thanks for the PR and for testing in your environment!
Enjoy your offline time☺️

I'd also recommend double checking that maintainers can edit your PR, in case Samuel finds time to review/release while you're gone. 😊

@maximgrigorov
Copy link

Hi there,

is it possible to merge this fix to main branch?

@akira
Copy link

akira commented Dec 22, 2023

Hello- thanks for the fix! Any thoughts on merging this? Would be great to have for my use cases! 🙇

@mernmic
Copy link
Contributor Author

mernmic commented Dec 27, 2023

Workaround while waiting for PR merge.

Initialize ArqRedis with a connection pool including your retry helper.

import redis
import arq

class WorkerSettingsBase:
    redis_pool = arq.ArqRedis(
        connection_pool=redis.asyncio.ConnectionPool.from_url(
            url=RedisSecrets.REDIS_URL,
            retry_on_timeout=True,
            retry_on_error=[
                redis.asyncio.TimeoutError,
                redis.asyncio.ConnectionError,
            ],
            retry=redis.asyncio.retry.Retry(
                backoff=CustomRedisBackoff(),
                retries=5,
            )
        )
    ),
   ....

if __name__ == "__main__":
    arq.run_worker(WorkerSettingsBase)

redis_pool parameter can be found in docs here:
https://arq-docs.helpmanual.io/#arq.worker.Worker

Example custom backoff class, we are using for more granular access and logging but you can use any redis retry helper

class CustomRedisBackoff(AbstractBackoff):
    def __init__(self, cap=0.512, base=0.008):
        """
        `cap`: maximum backoff time in seconds
        `base`: base backoff time in seconds
        """
        self.backoff = ExponentialBackoff(cap=cap, base=base)

    def compute(self, failures):
        sleep = self.backoff.compute(failures)
        logger.info(
            {
                "message": "redis connection error, triggering backoff",
                "failures": failures,
                "backoff": repr(self.backoff),
                "sleep (secs)": sleep,
            }
        )
        return sleep

docs on redis retry helpers:
https://redis-py.readthedocs.io/en/stable/retry.html

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Thanks so much for this, sorry for the long delay.

@samuelcolvin samuelcolvin enabled auto-merge (squash) April 1, 2024 10:36
@samuelcolvin
Copy link
Member

@JonasKs @Kludex we might be able to use redis's retry logic to remove our retry stuff - https://github.com/samuelcolvin/arq/blob/94cd8782b4f0764a17962186a349d32125cb98e3/arq/connections.py#L258-L286

@samuelcolvin samuelcolvin merged commit 8fe4fc5 into python-arq:main Apr 1, 2024
11 checks passed
@samuelcolvin
Copy link
Member

v0.26.0b1 is released, please try it, I'll release v0.26 at the end of the week, see #441.

@armicron armicron mentioned this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants