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.

Sign up for GitHub

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

Jump to bottom

feat: limit concurrent processing of thumbnail requests #9199

Merged
merged 1 commit into from
May 21, 2024
Merged

feat: limit concurrent processing of thumbnail requests #9199

merged 1 commit into from
May 21, 2024

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 16, 2024 •
edited
Loading

Description

Creating thumbnails can be quite intensive with respect to memory and cpu consumption.
Limiting the number of concurrent requests can help to relax this situation.

In case a request is rejected 429/Too Many Requests will be returned and clients can retry later.

Motivation and Context

Make OCIS/ByCS robust ...

How Has This Been Tested?

Screenshots (if appropriate):

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:

@update-docs Update Docs
Copy link

update-docs bot commented May 16, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@DeepDiver1975 DeepDiver1975 force-pushed the feat/thumbnail-request-limiter branch from a6e56c7 to c30fae4 Compare May 16, 2024 17:50
@mmattel
Copy link
Contributor

mmattel commented May 17, 2024

We need to add some text for the newly added thumbnail envvars in the thumbnails/readme.md, though I know you dont like writing stuff down 🤣

@DeepDiver1975 DeepDiver1975 force-pushed the feat/thumbnail-request-limiter branch from c30fae4 to 8c35a51 Compare May 17, 2024 06:58
@DeepDiver1975
Copy link
Member Author

We need to add some text for the newly added thumbnail envvars in the thumbnails/readme.md,

@mmattel done

@mmattel
Copy link
Contributor

mmattel commented May 17, 2024

Changes in the readme look good, many thanks !

@mmattel mmattel mentioned this pull request May 17, 2024
Closed
kobergj
kobergj approved these changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

lgtm

services/webdav/pkg/service/v0/service.go Show resolved Hide resolved
@DeepDiver1975
Copy link
Member Author

DeepDiver1975 commented May 17, 2024 •
edited
Loading

To some extend this could/should be added to all services?

  • 👍 yes - for sure!
  • 👎 NO - are you nuts!
  • 👀 Maybe - who cares ...

@kobergj
Copy link
Collaborator

kobergj commented May 17, 2024

Not sure if we want to this. Some services might benefit from that. But do we need it for all services?

Regardless, if we are aiming to use this is another service, it might make sense to call the envvar OCIS_MAX_CONCURRENT_REQUESTS right from the start. Then all these services can be configured via the same envvar.

@DeepDiver1975
Copy link
Member Author

Regardless, if we are aiming to use this is another service, it might make sense to call the envvar OCIS_MAX_CONCURRENT_REQUESTS right from the start. Then all these services can be configured via the same envvar.

🤷

Maybe someone with ops experience can help to answer that ... @wkloucek

@kobergj
Copy link
Collaborator

kobergj commented May 17, 2024

We already agreed to use less envvars. The conclusion was:
"If your envvar is used in only one service, prefix it with SERVICENAME. If it occurs in multiple services, prefix it with OCIS. Avoid having two envvars for the same configuration value."

kobergj
kobergj requested changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Please add changelog. I'd also recommend to change the envvar name so we dont need to deprecate it later...

@DeepDiver1975
Copy link
Member Author

Please add changelog

will do

I'd also recommend to change the envvar name so we dont need to deprecate it later...

OCIS_MAX_CONCURRENT_REQUESTS ?

But this is currently wrong as it will not limit requests for all of OCIS - might be missleading?

@kobergj
Copy link
Collaborator

kobergj commented May 21, 2024

But this is currently wrong as it will not limit requests for all of OCIS - might be missleading?

Yes thats true. So we have a tradeoff: Less Envvars <-> Better Envvar Naming. Really not sure whats better but I'd support team Less Envvars. Not critical though. If you prefer old name we can keep it.

@butonic
Copy link
Member

butonic commented May 21, 2024

Since this PR is only for the thumbnails service we can start with the THUMBNAILS_MAX_CONCURRENT_REQUESTS. When we need throtlling for other services we can still introduce OCIS_MAX_CONCURRENT_REQUESTS. THUMBNAILS_MAX_CONCURRENT_REQUESTS will overrule it and still exist anyway.

Stick to THUMBNAILS_MAX_CONCURRENT_REQUESTS

@DeepDiver1975 DeepDiver1975 force-pushed the feat/thumbnail-request-limiter branch from 8c35a51 to e79d632 Compare May 21, 2024 12:13
@DeepDiver1975 DeepDiver1975 requested a review from kobergj May 21, 2024 12:19
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
5.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@butonic butonic merged commit 0b965e3 into master May 21, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/thumbnail-request-limiter branch May 21, 2024 13:21
ownclouders pushed a commit that referenced this pull request May 21, 2024
@butonic
Merge pull request #9199 from owncloud/feat/thumbnail-request-limiter
79463d2 
feat: limit concurrent processing of thumbnail requests
butonic
butonic reviewed May 21, 2024
View reviewed changes
services/webdav/pkg/service/v0/service.go
Str("response_status", dlRsp.Status).
Msg("could not download thumbnail")
renderError(w, r, errInternalError("could not download thumbnail"))
renderError(w, r, newErrResponse(dlRsp.StatusCode, "could not download thumbnail"))
Copy link
Member

Choose a reason for hiding this comment

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

sabredav does not have a dedicated TooManyRequests exception. So we don't need to try and invent something here or in the reva webdav error handling.

We could omit a body ... and we should send a retry after header ... in a different PR.

This was referenced May 22, 2024
Closed
Merged
@micbar micbar mentioned this pull request May 29, 2024
Closed
mmattel added a commit that referenced this pull request Jun 3, 2024
@mmattel
Update thumbnails README.md
82e614e 
References: #9199 (feat: limit concurrent processing of thumbnail requests)

Content fix:
- If NO thumbnail is available, we return 404 (and not if it is avaialble)
- Make the resolution list multi-lined again for improved readbility
@mmattel mmattel mentioned this pull request Jun 3, 2024
Merged
@micbar micbar mentioned this pull request Jun 19, 2024
Closed
24 tasks
@micbar micbar mentioned this pull request Jul 17, 2024
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@butonic butonic butonic approved these changes

@kobergj kobergj kobergj approved these changes

@dragonchaser dragonchaser Awaiting requested review from dragonchaser

@wkloucek wkloucek Awaiting requested review from wkloucek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DeepDiver1975 @mmattel @kobergj @butonic