refactor(inkless): reuse storage metrics instance on cache #453
Conversation
jjaakola-aiven
left a comment
jjaakola-aiven
left a comment
There was a problem hiding this comment.
Naming of the metric is now unclear in the PR. It would be good to have a test to validate that metrics names are expected and not changed by mistake in future as the change will propagate to ops dashboards everywhere.
For example the cache size metrics name would be io.aiven.inkless.storage.wal-segment-cache.size, right?
| @Override | ||
| public void close() throws IOException { | ||
| metrics.close(); | ||
| cache.synchronous().cleanUp(); |
There was a problem hiding this comment.
👍
This is fine as long as there are no listeners attached to the cache. Otherwise may require blocking and waiting for the listeners to be called or using custom executor.
There was a problem hiding this comment.
Good catch. On a second thought I've leave this out of the PR as it's a bit out of context, and may add latency when restarting the broker. We can revisit if needed.
Instead of creating yet another metrics instance, reuse the storage metrics instance used by storage layer, as caching is a related module. This will change the metric name prefix from: `io.aiven.inkless.cache.caffeine` to `io.aiven.inkless.storage`
Adding missing headers
On a second thought, we could leave it as is as it may slow down shuting down the broker. Also, a bit outside the scope of the PR. Let's revisit if needed.
jeqo
force-pushed
the
jeqo/reuse-storage-metrics
branch
2 times, most recently
from
c74e1e3 to
46cfe26
Compare
jeqo
force-pushed
the
jeqo/reuse-storage-metrics
branch
from
46cfe26 to
da50d9c
Compare
|
@jjaakola-aiven added tests to ensure names stay, PTAL |
Instead of creating yet another metrics instance, reuse the storage metrics instance used by storage layer, as caching is a related module.
This will change the metric name prefix from:
io.aiven.inkless.cache.caffeinetoio.aiven.inkless.storageRelated fixes: