Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache metadata more #800

Open
twmb opened this issue Aug 6, 2024 · 2 comments · May be fixed by #896
Open

Cache metadata more #800

twmb opened this issue Aug 6, 2024 · 2 comments · May be fixed by #896
Labels
enhancement New feature or request has pr

Comments

@twmb
Copy link
Owner

twmb commented Aug 6, 2024

grafana/mimir#8886 (comment)

@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:

franz-go/pkg/kadm/metadata.go

Lines 432 to 436 in 6b61d17

func (cl *Client) listOffsets(ctx context.Context, isolation int8, timestamp int64, topics []string) (ListedOffsets, error) {
tds, err := cl.ListTopics(ctx, topics...)
if err != nil {
return nil, err
}

"""

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 twmb added the enhancement New feature or request label Aug 6, 2024
@pracucci
Copy link
Contributor

pracucci commented Aug 6, 2024

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).

@twmb
Copy link
Owner Author

twmb commented Oct 14, 2024

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 twmb mentioned this issue Jan 15, 2025
Open
13 tasks
twmb added a commit that referenced this issue Jan 22, 2025
@twmb
kgo: send all metadata requests through the internal mapped cache
7945c00 
Issue #800 was created as a follow up idea to strengthen caching of
metadata requests in the client. This pushes the mapped metadata caching
logic deeper into the guts of issuing metadata requests, so that no
caching is ever missed. The next commit will introduce a new API to
request potentially cached metadata.

For #800.
twmb added a commit that referenced this issue Jan 22, 2025
@twmb
kgo: add Client.RequestCachedMetadata
7ba4756 
This can be used to reduce the number of metadata requests issued. As
followup, kadm should almost globally use this function.

Closes #800.
twmb added a commit that referenced this issue Jan 22, 2025
@twmb
kadm: use cached metadata where possible
6eddacd 
As a follow up to #800, we convert kadm to using cached metadata
everywhere except for the actual Metadata function.

With a quick local test using `rpk group describe`, this brings the
prior 4 metadata requests down to 1.
@twmb twmb linked a pull request Jan 22, 2025 that will close this issue
Open
@twmb twmb added the has pr label Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request has pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

kgo: add Client.RequestCachedMetadata twmb/franz-go
2 participants
@twmb @pracucci