franz-go icon indicating copy to clipboard operation
franz-go copied to clipboard

Cache metadata more

Open twmb opened this issue 1 year ago • 2 comments

https://github.com/grafana/mimir/pull/8886#issuecomment-2265802337

@pracucci to reply to your latest message in the thread -- if we strengthen the caching within franz-go itself, then it addresses caching the metadata request you mention,

""" The pt1 of the PR description refers to the Metadata request issued to discover the partitions: https://github.com/twmb/franz-go/blob/6b61d17383b1e0c766143118efa39f1f320a3b2b/pkg/kadm/metadata.go#L432-L436 """

My proposal covers caching that^^, at which point the only extra caching your PR would provide is the 5 extra seconds (your PR uses 15s caching, but also elsewhere in Mimir you use 10s MetadataMinAge)

twmb avatar Aug 06 '24 14:08 twmb

From the original comment, you proposed 3 options:

  • Always use the mapped metadata cache for user issued Metadata requests
  • Introduce a new API, RequestCachedMetadata(req *kmsg.MetadataRequest, expiry time.Duration) (*kmsg.MetadataResponse, error). If the metadata exists and is cached for less than expiry, return it. If not, issue the metadata request (and force the response through the cache)
  • Introduce a new API, UseCache(context.Context) context.Context that can be used to query any internal cache when manually issuing a metadata request

My vote is the first or second bullet point. I think making these changes would avoid the need for this PR, but I'm not positive (not looking too closely).

As a user I would be prefer to be in control whether to use the cache and not. I guess option 1 and 2 wouldn't allow to have such control for an high-level request like ListOffsets() but, on the other side, I have no idea about option 3 complexity (which is also your least preferred one).

pracucci avatar Aug 06 '24 14:08 pracucci

The more I look at this, the more I think this needs to be done via (2) or (3), not (1). Saving for the next next release.

twmb avatar Oct 14 '24 23:10 twmb

This is released.

twmb avatar May 08 '25 17:05 twmb