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

warmup: evict sub command may remove stage incorrectly #5095

Merged
merged 3 commits into from
Aug 23, 2024
Merged

Conversation

jiefenghuang
Copy link
Contributor

@jiefenghuang jiefenghuang commented Aug 19, 2024

Add a new interface to clear the read cache.

@jiefenghuang jiefenghuang changed the title fix: warmup may remove stage incorrectly fix(warmup): evict sub command may remove stage incorrectly Aug 21, 2024
@@ -1005,6 +1025,7 @@ func expandDir(pattern string) []string {
type CacheManager interface {
cache(key string, p *Page, force, dropCache bool)
remove(key string)
removeReadCache(key string) // remove read cache only
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to add a flag in remove(key)

@davies davies changed the title fix(warmup): evict sub command may remove stage incorrectly warmup: evict sub command may remove stage incorrectly Aug 22, 2024
@polyrabbit
Copy link
Contributor

Kind reminder - when file is not uploaded to S3, it's dangerous to delete it from cache folder, especially when user enabled upload-delay or upload-hours features. The read procedure only checks file existence in cache folder and S3, not checking stage folder, so user will get an EIO error.

I suggest the evict subcommand should just ignore staging files.

Signed-off-by: jiefenghuang <[email protected]>
@davies davies merged commit 293f224 into main Aug 23, 2024
39 checks passed
@davies davies deleted the fix/cache branch August 23, 2024 09:56
SandyXSD pushed a commit that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants