Make all the caching components use snappy

[gouthamve]

We have an easy way to add snappy compression to caches, and we do it for the index-read-cache. But we should add it to other caches, specially the frontend-results cache.

We had it in the frontend-cache, but we lost it in this refactoring by mistake: #1437

[RajatVaryani]

@gouthamve Can I work on this?

[codesome]

@RajatVaryani sure go ahead!

[yeya24]

Hi, I want to work on supporting snappy cache in the query-frontend, since Thanos can also benefit from it.

Now my idea is to add a EnableSnappy config to ResultsCacheConfig and then modify the NewResultsCacheMiddleware function.

func NewResultsCacheMiddleware(
	logger log.Logger,
	cfg ResultsCacheConfig,
	splitter CacheSplitter,
	limits Limits,
	merger Merger,
	extractor Extractor,
	cacheGenNumberLoader CacheGenNumberLoader,
	shouldCache ShouldCacheFn,
	reg prometheus.Registerer,
) (Middleware, cache.Cache, error) {
	c, err := cache.New(cfg.CacheConfig, reg, logger)
	if err != nil {
		return nil, nil, err
	}
	if cfg.EnableSnappy {
		c = cache.NewSnappy(c, logger)
	}
        ...
}

If you think this way looks ok, I can do some performance testing and open a pr.

[pracucci]

Why not directly passing cache.NewSnappy() to NewResultsCacheMiddleware()?

[yeya24]

Hi @pracucci, did you mean to always use cache.NewSnappy in NewResultsCacheMiddleware()? My idea is to make snappy configurable since if only fifo cache is enabled, then it is not necessary to use snappy.

[pracucci]

We should allow to configure which compression should be used, but I'm wondering if we should do it in the parent caller to NewResultsCacheMiddleware() and not in the NewResultsCacheMiddleware() itself.

[yeya24]

I still think that adding a EnableSnappy config to ResultsCacheConfig is better since we don't need to update the function signatures. I will create a pr and we can talk there.

[ethervoid]

I think this Issue was closed partially by this PR #3217 . In what more places would you think adding Snappy would be useful?

[pracucci]

I think this Issue was closed partially by this PR #3217 . In what more places would you think adding Snappy would be useful?

I think we're good with the query-frontend for now. Going to close the issue.