Unshared blockstore #150
Unshared blockstore #150
Conversation
|
@aschmahmann - any objections to running this in prod for the weekend? we don't see a meaningful increase in block requests as a result of this change. |
|
Nope. We're not really serving traffic to anyone so even prod is effectively experimentation. If so might want to deploy sooner so it can be reverted before the weekend in case there's a problem. Saturn folks (e.g. @guanzo) might care about prod timing |
| } else { | ||
| return nil, nil, errors.New("failed to get notifier") | ||
| } | ||
| bstore, err := NewCacheBlockStore(1024) |
There was a problem hiding this comment.
🤔 @willscott @aarshkshah1992 @aschmahmann
Just a small ask that if we make breaking(?) change like this, we should also break (rename) metrics, just to be safe. Otherwise, we end up with confusing Grafana history making it impossible to reason about the system 😅
It also avoids situations where metric means different things when a different backend is used (block vs car).
To illustrate, the meaning of blockstore_cache_hit and blockstore_cache_requests changed, it is no longer "global hit/miss", but "hit per single http request". BUT, on _existing_Grafana it is graphed in the same place, and looks like we've only lost about ~10% of cache hits (but these are no longer across all requests, but per every single request):
|
in case this helps next week, this change exploded memory usage and prod seems to OOM and get restarted when that happens:
|
This reverts commit edb5cd4.
This removes the complexity of notifier / shared context between different fetches.
If blocks from a car are not in the order they are requested from the gateway, blocks will be immediately queried to fill any missing gaps.