BloomShipper: add cache for downloaded blocks #11394
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lesystem. Signed-off-by: Vladyslav Diachenko <[email protected]>
github-actions
bot
added
the
type/docs
|
Trivy scan found the following vulnerabilities:
|
Signed-off-by: Vladyslav Diachenko <[email protected]>
chaudum
reviewed
Dec 7, 2023
Comment on lines
+321
to
+323
| Close: func() error { | ||
| return deleteFolder(blockDirectory) | ||
| }, |
There was a problem hiding this comment.
I don't think that we want to delete the complete directory containing the block files every time we are done reading and call Close().
There was a problem hiding this comment.
Ok, I think I understand now why you implemented it like this.
There was a problem hiding this comment.
If we do not use the cache, then we can not control the total size of the files on FS, so, it's better to remove it once we complete the processing.
in our case, we will always use the cache, but some OSS users might have it disabled (default state)
pkg/storage/stores/shipper/bloomshipper/block_downloader_test.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/bloomshipper/block_downloader_test.go
Outdated
Show resolved
Hide resolved
… use the abstraction instead of using 'if' statements in the code. Signed-off-by: Vladyslav Diachenko <[email protected]>
chaudum
approved these changes
Dec 13, 2023
# Conflicts: # pkg/storage/stores/shipper/bloomshipper/block_downloader.go # pkg/storage/stores/shipper/bloomshipper/block_downloader_test.go
Signed-off-by: Vladyslav Diachenko <[email protected]>
rhnasc
pushed a commit
to inloco/loki
that referenced
this pull request
Apr 12, 2024
adapted embeddedcache.go to store downloaded bloom blocks on local filesystem. **What this PR does / why we need it**: This cache will be used by bloom-gateway to not download the blocks each time. In the cache we store the objects(`cachedBlock` struct) that contain: 1. `blockDirectory`: the `path` to the directory where the block was extracted 2. `activeQueriers`: thread-safe counter of active block users. This field is important because we do not want the directory to be removed while it's in use. 3. rest fields are needed for service needs When the downloadingWorker receives this entity, it increases the counter `activeQueriers` and creates blockQuerier wrapper that has `Close` function that will decrease the counter once blockQuerier is not needed anymore. When the cache entry is being removed from the cache, the cache calls the function `removeDirectoryAsync` which asynchronously removes the block's folder. This function checks every `Xms` if there are still active block queriers and once `activeQueriers` count is `0` the folder will be removed. Also, there is a timeout in this function, and once the timeout is reached, the folder will be force removed. **Special notes for your reviewer**: * If the cache is disabled, then the blocks will be downloaded each time the block is requested. * If the cache is used, the folder will be deleted by embeddedCache when it reaches memory size limit or items count limit. Otherwise, the block's folder will be deleted when `Close` function is called. **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a) --------- Signed-off-by: Vladyslav Diachenko <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adapted embeddedcache.go to store downloaded bloom blocks on local filesystem.
What this PR does / why we need it:
This cache will be used by bloom-gateway to not download the blocks each time.
In the cache we store the objects(
cachedBlockstruct) that contain:blockDirectory: thepathto the directory where the block was extractedactiveQueriers: thread-safe counter of active block users. This field is important because we do not want the directory to be removed while it's in use.When the downloadingWorker receives this entity, it increases the counter
activeQueriersand creates blockQuerier wrapper that hasClosefunction that will decrease the counter once blockQuerier is not needed anymore.When the cache entry is being removed from the cache, the cache calls the function
removeDirectoryAsyncwhich asynchronously removes the block's folder. This function checks everyXmsif there are still active block queriers and onceactiveQuerierscount is0the folder will be removed.Also, there is a timeout in this function, and once the timeout is reached, the folder will be force removed.
Special notes for your reviewer:
Closefunction is called.Checklist
CONTRIBUTING.mdguide (required)CHANGELOG.mdupdatedadd-to-release-noteslabeldocs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR