-
Notifications
You must be signed in to change notification settings - Fork 45
Feat Automatic cache purge #655
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
|
commit: |
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'm not entirely sure about naming here. If you have better ideas
packages/cloudflare/src/api/durable-objects/bucket-cache-purge.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/api/durable-objects/bucket-cache-purge.ts
Outdated
Show resolved
Hide resolved
`, | ||
tags.map((row) => row.tag) | ||
); | ||
if (tags.length < 100) { |
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.
if (tags.length < 100) { | |
if (tags.length < MAX_NUMBER_OF_TAGS_PER_PURGE) { |
tags.map((row) => row.tag) | ||
); | ||
if (tags.length < 100) { | ||
// If we have less than 100 tags, we can stop |
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.
// If we have less than 100 tags, we can stop | |
// If we have less than MAX_NUMBER_OF_TAGS_PER_PURGE tags, we can stop |
let tags = this.ctx.storage.sql | ||
.exec<{ tag: string }>( | ||
` | ||
SELECT * FROM cache_purge LIMIT ${MAX_NUMBER_OF_TAGS_PER_PURGE} | ||
` | ||
) | ||
.toArray(); | ||
if (tags.length === 0) { | ||
// No tags to purge, we can stop | ||
// It shouldn't happen, but just in case | ||
return; | ||
} |
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.
Maybe you can move this at the beginning of the loop and adjust the while condition to avoid duplication?
// We should use it | ||
if (env.NEXT_CACHE_DO_PURGE) { | ||
const durableObject = env.NEXT_CACHE_DO_PURGE; | ||
if (!durableObject) { |
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.
This should always be defined (check on l38)
tags.map((row) => row.tag) | ||
); | ||
// Delete the tags from the sql table | ||
this.ctx.storage.sql.exec( |
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.
Do we want to delete only if internalPurgeCacheByTags
was successful?
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 guess we could do that, but it means we have to keep every single tag, it doesn't look like we can know if it only fails partially
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.
Actually keeping them might cause an issue, we could end up in a loop trying to purge something that somehow can't get purged. And if it happens it could end up blocking a lot of other tag from being invalidated
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 tested the cache purge API:
When there is an error, it could 200 with:
{
success: false,
errors: [
{
code: 1100,
message: 'Tag exceeds maximum length of 1024 characters: ....'
},
{ code: 1016, message: 'One or more cache purge errors' }
],
messages: [],
result: null
}
{
success: false,
errors: [
{
code: 1315,
message: 'Non-printable characters are forbidden in tags'
},
{ code: 1016, message: 'One or more cache purge errors' }
],
messages: [],
result: null
}
In this case we should delete the tags.
It could also be rate limited (HTTP Status 429)
{
success: false,
errors: [
{
code: 1134,
message: 'Unable to purge, rate limit reached. Please wait and consider throttling your request speed'
}
],
messages: [],
result: null
}
For this particular case, we should bail out and not delete the tags
This PR will allow to automatically purge Cache Api entry everywhere where we use them