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
[rest] Add caching for add-on resource #4107
Conversation
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
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.
Are you sure this works the way you expect it to do? Server-side changes to remote add-on services (e.g. a new add-on post in the forum) do not produce an AddonEvent
and will never be picked up until a system restart.
No, I’m not sure. Do you know a better way to detect changes? |
The remote add-on services cache their content, too. The refresh time is 15 min, so I guess an automatic expiry of the cache with half of that would probably mitigate the issue. |
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Thanks for the idea, I have just pushed that! FYI I switched from having a |
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@J-N-K Can you please have a look at this PR? |
|
||
public static CacheControl cacheControl() { | ||
CacheControl cc = new CacheControl(); | ||
cc.setNoCache(true); | ||
cc.setMustRevalidate(true); | ||
cc.setPrivate(true); | ||
return cc; | ||
} |
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.
I think haveing a static final CACHE_CONTROL
constant would be better than creating a new object every time. I also feel that the danger these setting might be modified is very low.
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.
Otherwise LGTM
This reverts commit 080bc9a.
@J-N-K Done 👍 Sign-Off is missing for the revert commit, hope that's okay. Otherwise I need to force push ... |
As there are no registry listeners to use, I decided to listen to
AddonEvent
s./cc @jimtng