Skip to content

Conversation

@mem
Copy link
Contributor

@mem mem commented Jun 3, 2025

The WithMinimumCacheTTL option is used to control when cache entries are removed from the cache. The existing code sets this value to 15 seconds, and if this option is not passed, that is the fallback value.

@mem mem requested a review from a team as a code owner June 3, 2025 00:55
@mem mem requested review from cinaglia and colin-stuart and removed request for a team June 3, 2025 00:55
@mem mem force-pushed the mem/add-minttl-option-for-exchange branch from 3946491 to fdc60bc Compare June 3, 2025 15:05
The WithMinimumCacheTTL option is used to control when cache entries are
removed from the cache. The existing code sets this value to 15 seconds,
and if this option is not passed, that is the fallback value.

Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
@mem mem force-pushed the mem/add-minttl-option-for-exchange branch from fdc60bc to f45a2ff Compare June 3, 2025 15:29
@vtorosyan vtorosyan requested review from gamab and mgyongyosi June 6, 2025 15:05

// WithMinimumCacheTTL allows setting the minimum amount of time that a cache
// entry must be valid for in order for it to be reused.
func WithMinimumCacheTTL(ttl time.Duration) ExchangeClientOpts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to hear your use case for making this configurable. The 15-second leeway acts as way to enforce that a token is purged from the cache a few seconds before its actual expiration. Would you actually like it to be purged sooner/later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke to @mem offline and got some context around this. I understand the reasoning behind it and think it's a reasonable change.

I did share some minor concern about the naming here as WithMinimumCacheTTL doesn't quite convey what this is meant to achieve. That said, I'm not sure I can provide a substantially better alternative. Some ideas:

  • WithExpirationBuffer
  • WithEvictionThreshold

Copy link
Member

@cinaglia cinaglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality-wise this looks good to me. I understand the need for it.

My only suggestions here would be to rename the option to something hopefully more self-explanatory. I'd also suggest providing more context around why this is needed, perhaps by providing a real-life example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants