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

Add retry delays #310

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add retry delays #310

wants to merge 1 commit into from

Conversation

goodspark
Copy link
Contributor

Factors in two possible ways Github can indicate a retry delay. But then falls back to an exponentially increasing value. But in both cases, will cap the delay.

src/lib.rs Outdated Show resolved Hide resolved
@envp
Copy link
Contributor

envp commented Apr 17, 2023

Now that the repo is using tower, this PR can use tower::retry to add the retry with exponential backoff

@L1ghtman2k
Copy link
Contributor

L1ghtman2k commented Apr 24, 2023

Now that the repo is using tower, this PR can use tower::retry to add the retry with exponential backoff

For reference, octocrab already use tower::retry https://github.com/XAMPPRocky/octocrab/blob/main/src/service/middleware/retry.rs, but it has a very simple logic (3 retries), so maybe the changes should go there to implement exponential backoff

@XAMPPRocky
Copy link
Owner

but it has a very simple logic (3 retries), so maybe the changes should go there to implement exponential backoff

Yeah let's change that that type to use this flow. If @goodspark isn't available or doesn't have the time to change it, someone else is welcome to make as long as they include them as a co-author in the commit.

@goodspark
Copy link
Contributor Author

I can give it a whirl

@goodspark
Copy link
Contributor Author

This is gonna be a bit tricky with the current tower retry traits. I think I'd recommend waiting until tower-rs/tower#682 lands so we can just leverage the changes to the retry policy trait and the backoff.

Key complication is that I need to chain two futures together (the future from calling sleep and the future for returning the next policy). And the typing for that is turning out to be a nightmare.

@goodspark
Copy link
Contributor Author

Actually, we could possibly just ref the commit in the dependency to tower. Let me try that.

@goodspark
Copy link
Contributor Author

Pushed a change that uses the new retry policy stuff. Basically lets us mutate the policy object instead of returning a new one. This simplifies the returned future type to just the tokio::time::Sleep future, since we'll just mutate the RetryConfig's fields.

But this doesn't compile. I think it's bc Rust is getting confused about which version of tower-layer is being used (since I'm telling it pull down tower from a repo+commit, which includes tower-layer). I'm not sure how to fix that, though.

Factors in two possible ways Github can indicate a retry delay. But then
falls back to an exponentially increasing value. But in both cases, will
cap the delay.
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.

4 participants