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

Cache: purge rtd-addons when a new version is enabled & built #11489

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 22, 2024

Note that I wasn't able to write a test case because readthedocsext is not installed in our test suite, so I cannot check this task is called.

Closes #11291

Note that I wasn't able to write a test case because `readthedocsext` is not
installed in our test suite, so I cannot check this task is called.

Closes #11291

This comment was marked as off-topic.

@humitos humitos marked this pull request as ready for review November 18, 2024 16:57
@humitos humitos requested a review from a team as a code owner November 18, 2024 16:57
@humitos humitos requested a review from stsewd November 18, 2024 16:57
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

This is purging all responses from addons, we want to purge only the response from a given project/version.

You may want to add this at https://github.com/readthedocs/readthedocs-ext/blob/main/readthedocsext/cdn/providers/base.py#L66-L69 instead.

@humitos
Copy link
Member Author

humitos commented Nov 19, 2024

Good point! I added the project.slug to the tags and it worked 👍🏼

In [1]: from readthedocsext.cdn.tasks import purge_tags

In [2]: purge_tags(["test-builds", "rtd-addons"])
readthedocs[28239]: [info     ] Purged Cloudflare cache.       [readthedocsext.cdn.providers.cloudflare:115] tags=['test-builds', 'rtd-addons']

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

This should be in rtd-ext, where we already have all the purging logic for all cases (old flyout).

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

I don't follow you. How do you want to call the purge logic from .org? What function/method in particular?

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

I don't follow you. How do you want to call the purge logic from .org? What function/method in particular?

We already call the purge method from .org (using a signal), you just need to add the tag in rtd-ext.

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

What signal? I think you need to be more explicit here if you want me to follow you.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

You just need to add the new tag in https://github.com/readthedocs/readthedocs-ext/blob/main/readthedocsext/cdn/providers/base.py#L66-L69, we already call that purge method from .org when needed.

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

we already call that purge method from .org when needed

Well, that's my point. We are not calling the purge method from .org when needed. We need to purge the cache after a version is enabled and built. That's not currently happening.

BTW, you didn't answer my previous question: "What signal?"

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

We are not calling the purge method from .org when needed. We need to purge the cache after a version is enabled and built.

We are, that's how the old flyout is purged.

# This signal is used for purging the CDN.
files_changed.send(
sender=Project,
project=version.project,
version=version,
)

https://github.com/readthedocs/readthedocs-ext/blob/45089d0ba4ee2d9f3e8471c4444601ad060cf587/readthedocsext/cdn/signals.py#L30-L38

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

I don't want to purge the addons endpoint every time a file is changed in a version.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

I don't want to purge the addons endpoint every time a file is changed in a version.

That signal is triggered when a version is built.

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

It's the same. I don't want to purge the endpoint every time a version is built. I only want it when it was just enabled and built. That means, the first time the version is built after being enabled.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

Your current call is purging the cache on every build

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

You still want to purge the cache on every build, if files changed, FTD needs to return a fresh response.

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

Your current call is purging the cache on every build

No. It only call the purge when the version was not built previously and now is built.

You still want to purge the cache on every build, if files changed, FTD needs to return a fresh response.

You are trying to solve a different problem here, and it's outside the scope of this PR. File Tree Diff only works on PR builds for now and we use a different approach there.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2024

There are more cases where we are serving stale information from addons, get_version_cache_tags already covers all those cases (a new translation is another case). Here we are trying to solve a very specific case instead of the general one (which was already solved for the old flyout).

@humitos
Copy link
Member Author

humitos commented Nov 20, 2024

That's a good argument. You could have started there.

However, that's a completely different conversation. We will be switching from purging the cache "once in a while" (in very specific situations) to "on every build" which is a bigger change here. I'm happy to discuss that, but that's out the scope of this PR definitely.

The response for the old flyout had a lot less data than addons, so we may want to be caution here with when we are purging the cache.

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.

Addons: activating a version doesn't purge the cache
2 participants