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

Rotate auth token in backoff_handler #131

Merged
merged 11 commits into from
May 23, 2022
Merged

Conversation

laurentS
Copy link
Contributor

This PR fixes #129 to handle the edge case described in it.

The limit to this is that the token will be rotated on every RetriableAPIError with this code, which may not be desirable.
I don't see any obvious way to prevent this, short of throwing a different exception upon rate limit errors, which would require modifying request_decorator. Happy to hear any other ideas :)

@laurentS laurentS requested a review from ericboucher May 19, 2022 15:15
@ericboucher
Copy link
Contributor

I am SHOCKED that the backoff library doesn't return the response or even just its status code...

Anyways, investigating this I found out that we were missing a key component, actually updating to the SDK that has this new featured 😉

@ericboucher
Copy link
Contributor

@laurentS looks like upgrading the SDK broke tests around collaborators, we probably need to change our implementation of alternative_sync_chidren

@ericboucher
Copy link
Contributor

ericboucher commented May 20, 2022

I opened - litl/backoff#158

Noting that the SDK should upgrade to backoff 2.0.1 to get better typing @edgarrmondragon ;)

@edgarrmondragon
Copy link
Member

edgarrmondragon commented May 20, 2022

@ericboucher

Noting that the SDK should upgrade to backoff 2.0.1 to get better typing @edgarrmondragon ;)

Yup, we're blocked by transferwise/pipelinewise-singer-python#61

I opened - litl/backoff#158

What you want is already possible in backoff 2.0 with the (undocumented) backoff.runtime: https://github.com/litl/backoff/blob/92e7b6e910accee30118a6f3d2cef4c62caa38f5/tests/test_integration.py#L32-L37.

We ended up porting it to the SDK (because of the blocker above) as RESTStream.backoff_runtime and there's an example here: https://sdk.meltano.com/en/latest/code_samples.html#custom-backoff that accesses the response object via the RetriableAPIError instance 🙂

Wdyt?

@laurentS
Copy link
Contributor Author

I found a way to rotate the token only on relevant errors. It's probably not the most reliable solution, but should be fine as a temp fix until the PRs mentioned above are merged.

@sonarcloud
Copy link

sonarcloud bot commented May 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ericboucher ericboucher merged commit f152666 into main May 23, 2022
@ericboucher ericboucher deleted the fix-backoff-on-rate-limit-error branch May 23, 2022 01:57
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.

Update rate limit handling to use new sdk feature
3 participants