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: Retry writing lease when error code 429 (too many requests). #2773

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

vrongmeal
Copy link
Contributor

This should fix the issue. We are retrying here as suggested by GCS since writing the same object again and again is rate limited to once per second.

Another thing that might help is writing to a new "temp" object every time (instead of using the same one for the same process).

Fixes: https://github.com/GlareDB/cloud/issues/2746

This should fix the issue. We are retrying here as suggested by GCS
since writing the same object again and again is rate limited to once
per second.

Another thing that might help is writing to a new "temp" object every
time (instead of using the same one for the same process).

Fixes: https://github.com/GlareDB/cloud/issues/2746

Signed-off-by: Vaibhav <vrongmeal@gmail.com>
Comment on lines +348 to +351
// TODO: Generate a new temp path everytime? This will definitely avoid
// the rate limitation over over-writing the same object here and might
// just be enough to delay the "renaming" process so we don't exceed
// the limit there as well.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scsmithr how do you feel about this?

Copy link
Member

Choose a reason for hiding this comment

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

I think that'd be reasonable, we can just generate a temp path every time. For some reason, I had read the original error message thinking that we were errorring on writing to the actual lease, and not the temp. But if it's just the temp, then this seems good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged this PR before reading this. Will open another to add this as well.

Comment on lines +348 to +351
// TODO: Generate a new temp path everytime? This will definitely avoid
// the rate limitation over over-writing the same object here and might
// just be enough to delay the "renaming" process so we don't exceed
// the limit there as well.
Copy link
Member

Choose a reason for hiding this comment

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

I think that'd be reasonable, we can just generate a temp path every time. For some reason, I had read the original error message thinking that we were errorring on writing to the actual lease, and not the temp. But if it's just the temp, then this seems good.

// Error code 429 is not handled by object_store's retry
// configuration. We should maybe upstream to catch this
// and retry properly.
if store == &"GCS" && source.contains("429") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I worry about the fragility of this little block, (in terms of the matching/destructuring of the error as well as the constants/string parsing of the error.

@vrongmeal vrongmeal merged commit 7450e09 into main Mar 12, 2024
25 checks passed
@vrongmeal vrongmeal deleted the vrongmeal/lease-retries branch March 12, 2024 16:20
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.

3 participants