-
Notifications
You must be signed in to change notification settings - Fork 20
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 @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):
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.