-
Notifications
You must be signed in to change notification settings - Fork 670
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
feat: try reading shard_cache if prefetcher blocking_get returns None #8287
feat: try reading shard_cache if prefetcher blocking_get returns None #8287
Conversation
self.metrics.prefetch_retry.inc(); | ||
self.read_from_db(hash)? | ||
if let Some(value) = self.shard_cache.get(hash) { | ||
value |
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.
@jakmeier do you think it is worth introducing another metric for this case, something like:
self.metrics.prefetch_shared_cache_after_pending.inc();
also suggestions for a better name are welcome :)
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.
Hm, yeah I think we can add it. It's exactly these weird corner cases that are hard to debug, having insight with metrics might turn out useful.
For the name, do you mean shard_cache
or shared_cache
? I think prefetch_shard_cache_after_pending
makes more sense to me.
But actually, how about prefetch_pending_conflict
, or prefetch_pending_conflict_hit
, or prefetch_main_thread_conflict
or even just prefetch_conflict
?
My thinking is: The request was pending but instead of the expected prefetch-hit afterwards, there was a conflict with "another main thread" that also lead to a hit, but the hit was unexpectedly in shard cache. You name also work though, these are just some suggestions.
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.
it meant to be shard_cache
, sorry for the typo
prefetch_conflict
definitely makes to me, let's use it
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.
LGTM, thanks for also finishing the second half of the issue
This is the second part of #7723: avoid an unnecessary storage lookup from the main thread in the scenario of forks.