Refactor: extract parquet shard cache to parquetutil pkg #7146
Conversation
SungJin1212
changed the title
pkg/util/parquetutil/cache.go
Outdated
| func newCacheMetrics(reg prometheus.Registerer) *cacheMetrics { | ||
| return &cacheMetrics{ | ||
| hits: promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | ||
| Name: "cortex_parquet_queryable_cache_hits_total", |
There was a problem hiding this comment.
should we remove queryable from the metric name? Or you make cortex_parquet_queryable as a prefix to those metrics
There was a problem hiding this comment.
I see you already have name as a label then we need to remove the parquet queryable prefix in metric name. Otherwise you make prefix configurable
pkg/util/parquetutil/cache.go
Outdated
| } | ||
|
|
||
| type CacheConfig struct { | ||
| ParquetQueryableShardCacheSize int `yaml:"parquet_queryable_shard_cache_size"` |
There was a problem hiding this comment.
I think it should be parquet shard cache size not parquet queryable cache size... It is a cache for parquet shard which exists in parquet store gateway as well
SungJin1212
force-pushed
the
Refactor-move-parquet-shard-cache-to-dedicated-pkg
branch
from
f64b0a4 to
031650a
Compare
|
@yeya24 |
SungJin1212
force-pushed
the
Refactor-move-parquet-shard-cache-to-dedicated-pkg
branch
3 times, most recently
from
cc9eba7 to
ea253e3
Compare
| func NewCache[T any](cfg *CacheConfig, name string, reg prometheus.Registerer) (CacheInterface[T], error) { | ||
| if cfg.ParquetShardCacheSize <= 0 { | ||
| return &noopCache[T]{}, nil | ||
| } |
There was a problem hiding this comment.
This is a bit weird as the function name seems to be generic but it checks cfg.ParquetShardCacheSize which seems not generic.
Maybe worth refactoring again to have NewParquetShardCache to create the cache for parquet shards and move the logic there
| size: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "cortex_parquet_cache_item_count", | ||
| Help: "Current number of cached parquet items", | ||
| }, []string{"name"}), |
There was a problem hiding this comment.
Can u add the metric name change to the changelog? I think we are ok to rename metrics since they are still experimental but need to document it
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Refactor-move-parquet-shard-cache-to-dedicated-pkg
branch
2 times, most recently
from
a023b18 to
08037f5
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Refactor-move-parquet-shard-cache-to-dedicated-pkg
branch
from
08037f5 to
1e041e7
Compare
| func (cfg *CacheConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { | ||
| f.IntVar(&cfg.ParquetShardCacheSize, prefix+"parquet-shard-cache-size", 512, "[Experimental] Maximum size of the Parquet shard cache. 0 to disable.") | ||
| f.DurationVar(&cfg.ParquetShardCacheTTL, prefix+"parquet-shard-cache-ttl", 24*time.Hour, "[Experimental] TTL of the Parquet shard cache. 0 to no TTL.") | ||
| cfg.MaintenanceInterval = defaultMaintenanceInterval |
There was a problem hiding this comment.
I didn't realize that we also renamed the flags here. Also the configs above. Let's add those to changelog as well
There was a problem hiding this comment.
yeah, I made a PR #7165. I added config as well.
2 participants
This PR extracts the parquet shard cache implementation into a new dedicated package
pkg/util/parquetutil.This refactoring allows the shard cache logic to be reused by other components.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]