refactor(storage-manager): implement cache for History::Latest
#1454
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
|
PR missing one of the required labels: {'internal', 'documentation', 'new feature', 'bug', 'breaking-change', 'dependencies', 'enhancement'} |
plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs
Outdated
Show resolved
Hide resolved
The changes introduced in #1367 were actually redundant (and incomplete): the method `is_latest` was checking that a received Sample was more recent than what is currently stored. This commit removes the redundant checks and implements a correct caching strategy by leveraging both approaches. The changes consist mostly in: 1. Changing the outer test by continuing early if the Sample is detected as deleted. This allows lowering the overall indentation of the `process_sample` method by one level. 2. Modifying the behaviour of the `is_latest` method to check the cache first and only query the Storage if there is a cache miss. To help make potential concurrency issues visible, the method `is_latest` was renamed to `guard_cache_if_latest` as the lock on the Cache should only be released when the Sample has been processed *and* the Cache updated. Additionally, the cache is filled at initialisation. The `read_cost` configuration parameter was removed as it was not used and made obsolete with this change. * plugins/zenoh-backend-example/src/lib.rs: * plugins/zenoh-backend-traits/src/lib.rs: * plugins/zenoh-plugin-storage-manager/src/memory_backend/mod.rs: removed the obsolete `read_cost` parameter. * plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs: - Populate the Cache at initialisation. - Updated the call to `StorageService::start` to also pass the Cache. * plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs - Updated the call to `StorageService::start` to also pass the Cache. - "Reversed" the logic when checking if a Sample is deleted: if it is continue to the next one. This allows reducing the indentation level. - Keep the guard over the cache if a Sample is more recent and only release it once it has been updated (after the Sample has been successfully processed). Signed-off-by: Julien Loudet <[email protected]>
J-Loudet
force-pushed
the
refactor/storage-manager/cache-latest
branch
from
2bb5cb3 to
3295c82
Compare
|
@J-Loudet, Cache implementation looks good. |
|
Relevant internal tests: https://github.com/ZettaScaleLabs/zenoh-tests/actions/runs/10944755270 |
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.
The changes introduced in #1367 were actually redundant (and incomplete): the method
is_latestwas checking that a received Sample was more recent than what is currently stored.This commit removes the redundant checks and implements a correct caching strategy by leveraging both approaches. The changes consist mostly in:
process_samplemethod by one level.is_latestmethod to check the cache first and only query the Storage if there is a cache miss.To help make potential concurrency issues visible, the method
is_latestwas renamed toguard_cache_if_latestas the lock on the Cache should only be released when the Sample has been processed and the Cache updated.Additionally, the cache is filled at initialisation.
The
read_costconfiguration parameter was removed as it was not used and made obsolete with this change.plugins/zenoh-backend-example/src/lib.rs:
plugins/zenoh-backend-traits/src/lib.rs:
plugins/zenoh-plugin-storage-manager/src/memory_backend/mod.rs: removed the obsolete
read_costparameter.plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs:
StorageService::startto also pass the Cache.plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs
StorageService::startto also pass the Cache.