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

feat: return Retry-After header in case of 429/Too Many Requests on thumbnail WebDAV endpoint #9240

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

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 22, 2024

Description

#9199 (comment)

Related Issue

#9199 (comment)

Motivation and Context

#9199 (comment)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

This comment was marked as resolved.

Copy link

sonarcloud bot commented May 23, 2024

@butonic
Copy link
Member

butonic commented May 23, 2024

hm, yeah ... determining a retry after timeout is hard. 5min is pretty long ... I'd start with 30sec and use an exponential backoff.

Also, the current Throttle limit is global ... maybe sth like https://github.com/go-chi/httprate with a limit on real ip and endpoint? urg but that requires common state, which is why there is https://github.com/go-chi/httprate-redis

since this is something I expect to be tweakable ... should we just add an env var for this?

@DeepDiver1975
Copy link
Member Author

Also, the current Throttle limit is global ... maybe sth like https://github.com/go-chi/httprate with a limit on real ip and endpoint? urg but that requires common state, which is why there is https://github.com/go-chi/httprate-redis

But this is more for real rate limiting which could be used for some kind of far usage of the thumbnail service.
The existing implementation limits resource consumption of the thumbnail service.

Or am I miss-understanding your comment?

since this is something I expect to be tweakable ... should we just add an env var for this?

Two vars then? One for done and one for not done ....

hm, yeah ... determining a retry after timeout is hard. 5min is pretty long ... I'd start with 30sec and use an exponential backoff.

I didn't want to over-engineer this tbh 🙈

@micbar
Copy link
Contributor

micbar commented Jul 9, 2024

Please finish or close 😄

@DeepDiver1975 DeepDiver1975 deleted the feat/429-webdav-response branch July 9, 2024 09:35
@micbar micbar restored the feat/429-webdav-response branch July 9, 2024 09:37
@micbar
Copy link
Contributor

micbar commented Jul 9, 2024

I should have written "finish" 😬

@micbar micbar reopened this Jul 9, 2024
@micbar
Copy link
Contributor

micbar commented Jul 9, 2024

@butonic wants it -> please finish

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.

None yet

3 participants