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

fix: prevent repeated context expired errors #458

Merged
merged 5 commits into from
Feb 14, 2023
Merged

Conversation

enocom
Copy link
Member

@enocom enocom commented Feb 14, 2023

The Go Connector uses a rate limiter to prevent excessive API usage. When refresh attempts failed a few times, however, the Go Connector would continue to schedule new attempts which would be rate limited. The rate limited error would then result in another refresh attempt being scheduled. As a result, the Go Connector would report numerous "context deadline expired" errors which were in fact rate limiting errors.

This commit moves the rate limiter to the entry point of the refresh code. This change ensures that if there are refresh failures, there are not runaway attempts to refresh again. Instead, when an error occurs, a new refresh attempt will run only once the rate limiter allows it to run.

Fixes #384

The Go Connector uses a rate limiter to prevent excessive API usage.
When refresh attempts failed a few times, however, the Go Connector
would continue to schedule new attempts which would be rate limited. The
rate limited error would then result in another refresh attempt being
scheduled. As a result, the Go Connector would report numerous "context
deadline expired" errors which were in fact rate limiting errors.

This commit moves the rate limiter to the entry point of the refresh
code. This change ensures that if there are refresh failures, there are
not runaway attempts to refresh again. Instead, when an error occurs, a
new refresh attempt will run only once the rate limiter allows it to
run.
Copy link
Collaborator

@jackwotherspoon jackwotherspoon left a comment

Choose a reason for hiding this comment

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

LGTM, nice fix!

The timeout should be greater than the interval enforced by the rate
limiter to ensure there is time left over after being rate limited to
complete a refresh cycle.
@enocom enocom merged commit 7ffeafe into main Feb 14, 2023
@enocom enocom deleted the fix-expired-context branch February 14, 2023 20:47
enocom added a commit to GoogleCloudPlatform/alloydb-go-connector that referenced this pull request Feb 15, 2023
enocom added a commit to GoogleCloudPlatform/alloydb-go-connector that referenced this pull request Feb 15, 2023
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.

refresh failed: context deadline exceeded
2 participants