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

fix(cache:disk): fail when base directory does not exist #311

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

jeqo
Copy link
Contributor

@jeqo jeqo commented Jul 4, 2023

Currently, disk-based cache is only validating if directory exist to delete it and re-create child directories.

This approach is inconsistent with how other paths are defined in the plugin, requiring the directory to exist prior to the start of the plugin. The plugin does this to avoid messing with the existing permissions around the preparation of the directory.

Applying the fixes to fail if directory doesn't exist and some additional refactoring.

@jeqo jeqo requested a review from a team as a code owner July 4, 2023 12:10
Copy link
Contributor

@ivanyu ivanyu left a comment

Choose a reason for hiding this comment

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

LGTM in general, one small thing with the README

README.md Outdated
Comment on lines 113 to 116
#### Disk-based Chunk Cache

Requires a base directory (`path` configuration).
This directory is not created by the plugin, so it needs to be provided before starting the broker.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't put this paragraph in this section, because it's the design overview, not the usage documentation. We can either create the usage section with this paragraph and lots of TBD, or drop it for now until we start writing the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, created #312 to follow up, and dropped changes on README.md

@jeqo jeqo force-pushed the jeqo/fix-cache-disk-base-not-exist branch from 0f6739a to 0f44d80 Compare July 5, 2023 11:13
@jeqo jeqo requested a review from ivanyu July 5, 2023 11:14
@ivanyu ivanyu self-assigned this Jul 5, 2023
@ivanyu ivanyu merged commit 51ed2d5 into main Jul 5, 2023
@ivanyu ivanyu deleted the jeqo/fix-cache-disk-base-not-exist branch July 5, 2023 11:24
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.

2 participants