ethereum / web3.py

A python interface for interacting with the Ethereum blockchain and ecosystem.
http://web3py.readthedocs.io
MIT License
4.91k stars 1.68k forks source link

feat: Exponential backoff factor for HTTP request retries #3358

Closed hr98w closed 3 months ago

hr98w commented 3 months ago

What was wrong?

Related to Issue https://github.com/ethereum/web3.py/issues/3210

How was it fixed?

Make the backoff_factor for the ExceptionRetryConfiguration class work as an actual exponential "backoff factor" And Update the doc.

Reuse backoff_factor as the initial back off time

Introduce a new field exponential_factor

Todo:

Cute Animal Picture

Screenshot 2024-04-24 at 10 40 03
hr98w commented 3 months ago

@fselmo as per this issue https://github.com/ethereum/web3.py/issues/3210 I was considering add a new field initial_backoff_time and re-use backoff_factor as the exponential factor initially. But this will affect existing usage of field backoff_factor and might cause backward compatibility issue. Please help review the PR and share your thoughts.

fselmo commented 3 months ago

Hey @hr98w. Thanks for getting this started. I'm not sure that we need to add yet another configuration option here. If we make the sleep time itself exponential, the backoff factor can apply directly to that. Something like:

for i in range(self.exception_retry_configuration.retries):
    try:
        return make_post_request(
            self.endpoint_uri, request_data, **self.get_request_kwargs()
        )
    except tuple(self.exception_retry_configuration.errors) as e:
        if i < self.exception_retry_configuration.retries - 1:
            time.sleep(
                # the `backoff_factor` works as a factor on the inherent
               # exponential setup of 2**i
                self.exception_retry_configuration.backoff_factor * 2**i
            )
            continue
        else:
            raise e
return None

Thoughts on that?

hr98w commented 3 months ago

@fselmo sure, this also works.

I introduced a new variable to let users determine the exponential factor. Keeping that factor as 1 means no exponenial backoff. We can keep it simple here, will follow your advice.

fselmo commented 3 months ago

I introduced a new variable to let users determine the exponential factor. Keeping that factor as 1 means no exponenial backoff. We can keep it simple here, will follow your advice.

I hear you. That would allow for more configuration for sure. Exponential is pretty standard and keeping that factor low still allows for pretty quick succession if desired. If you're good with my simpler approach then let's go for it 👍🏼

fselmo commented 3 months ago

We should maybe decrease the default value here to 0.125? That would yield .125 -> .25 -> .5 -> 1 -> 2. That feels pretty good for JSON-RPC calls. Thoughts there?

hr98w commented 3 months ago

Starting from 0.5 seems too large for me. But i'm not sure for JSON-RPC calls, any latency stats you can share?

hr98w commented 3 months ago

@fselmo Updated the PR, please help review

hr98w commented 3 months ago

@fselmo Thanks for your comment! Got your point to make it easy to understand. Resolved them.

fselmo commented 3 months ago

Thanks again 👍🏼