-
Notifications
You must be signed in to change notification settings - Fork 740
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(cachingFilter: Allow disabling the content caching filter #1699
fix(cachingFilter: Allow disabling the content caching filter #1699
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, though I'd also be in favor of removing it entirely.
:P this way I can backport it ;) since feature flagged |
@Mergifyio backport release-1.30.x release-1.31.x release-1.32.x |
✅ Backports have been created
|
* fix(cachingFilter: Allow disabling the content caching filter * fix(cachingFilter: Allow disabling the content caching filter (cherry picked from commit 7e403b0)
* fix(cachingFilter: Allow disabling the content caching filter * fix(cachingFilter: Allow disabling the content caching filter (cherry picked from commit 7e403b0)
* fix(cachingFilter: Allow disabling the content caching filter * fix(cachingFilter: Allow disabling the content caching filter (cherry picked from commit 7e403b0)
https://github.com/Mergifyio backport release-1.28.x |
✅ Backports have been created
|
* fix(cachingFilter: Allow disabling the content caching filter * fix(cachingFilter: Allow disabling the content caching filter (cherry picked from commit 7e403b0)
@jasonmcintosh do you have any examples of what this breaks? Curious if any issues we see tally. |
The biggest problem I've PERSONALLY seen right is situations where gate just fails BADLY on these responses on LARGE pipelines. We've got traces showing orca respond in 2 seconds, then gate takes 80 seconds to do this. It's ENTIRELY in this filter eating up response times. SO there's some of this that's... potentially "scaling" issues or similar on gate to match orca capacities, but it's definitely been painful memory usage as well because of how this operates. There are other areas this breaks when using different UI libs as well. @jvz can probably talk more to some of those impacts. |
Plus also, why not backport to 1.29/1.30/1.31/1.32? |
Cause I missed that in my copy/paste ;)
|
https://github.com/Mergifyio backport release-1.29.x |
✅ Backports have been created
|
* fix(cachingFilter: Allow disabling the content caching filter * fix(cachingFilter: Allow disabling the content caching filter (cherry picked from commit 7e403b0)
The original theory on this I think was eTag support. BUT there's some major gotchas around that. |
The caching filter breaks async controllers that use reactive streams or coroutines for one. |
We see this caching filter break things BADLY. Ability to disable it, defaults to leaving it on.