-
Notifications
You must be signed in to change notification settings - Fork 51
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
Remove async cache provider #1720
Conversation
This pull request has been linked to Shortcut Story #8277: Simplify shard cache layer. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1720 +/- ##
==========================================
+ Coverage 82.16% 82.21% +0.05%
==========================================
Files 334 334
Lines 19537 19537
==========================================
+ Hits 16052 16062 +10
+ Misses 3485 3475 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4bb9d81
to
581b429
Compare
@@ -79,19 +85,20 @@ impl ShardReaderProvider for UnboundedShardReaderCache { | |||
fn load_all(&self) -> NodeResult<()> { | |||
let mut cache = self.write(); | |||
let shards_path = self.shards_path.clone(); | |||
debug!("Recovering shards from {shards_path:?}..."); | |||
|
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.
load_all
differed between the sync & async version:
- The async version loaded all shards and then grabbed the lock to insert all shards into cache.
- The sync version held the lock while loading all shards.
This version behaves like the sync version and thus blocks the cache until everything is loaded. It seems safer but potentially a long block. The async version could be prone to race-conditions if a shard is opened while load_all
is running. A third alternative would be to grab a write lock for each iteration of the loop, so other consumers get the chance to interact with the cache (albeit with high contention).
581b429
to
91f1367
Compare
91f1367
to
37bba75
Compare
Description
spawn_blocking
).How was this PR tested?
Tests and basic tinkering. Will probably require some performance testing in stage.