Skip to content

Context-aware instance manager #1352

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Context-aware instance manager #1352

wants to merge 1 commit into from

Conversation

wbrowne
Copy link
Contributor

@wbrowne wbrowne commented Jul 24, 2025

An alternative to #1230 that implements TTL for datasource instances, but decides that logic at runtime via a feature toggle

Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

I think this makes sense! We just need to remember to incorporate 3ad2374 in before merging the parent pr

@github-project-automation github-project-automation bot moved this from 📬 Triage to 🔬 In review in Plugins Platform / Grafana Community Jul 24, 2025
cache.OnEvicted(func(_ string, value interface{}) {
ci := value.(CachedInstance)
if disposer, valid := ci.instance.(InstanceDisposer); valid {
time.AfterFunc(disposeTTL, func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwysiu IIUC we may no longer need to make use of the disposeTTL here - can you confirm if my understanding is correct?

When the instanceTTL expires, the instance will be inaccessible via Get() but still exist in memory. When the cleanup interval comes around, as long as that happens a reasonable amount of time after the instanceTTL, then we don't need to worry about anyone have a hold of the instance so we can dispose right away. Also once the instanceTTL expires, there's no way to revive it.

Copy link
Contributor

@iwysiu iwysiu Jul 24, 2025

Choose a reason for hiding this comment

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

I think we should be ok to remove it, but do we think there's a possibility of the following/do we care if it happens?

  1. instance fetched
  2. instance expired
  3. instance cleared (theoretically possible if it just happens at a regular pace?)
  4. instance used

I think that's the only possible problem seres of events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this scenario - are you imagining that in order for 4. to occur, there would have to be a long-lived piece of code that is running where the instance is being used?

The instance is fetched every time a query is made (QueryData, CheckHealth, etc.), then expires after no usage for 1hr. It exists in memory for another hour before being disposed, but cannot be fetched again in that timeframe since it's expired according to the cache. The only way I see 4 occurring is if some code still has access to the instance elsewhere?

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

The changes makes sense to me 👍 I'd target this PR against main though, to make it clear what's the new code.

@wbrowne wbrowne changed the base branch from iwysiu/ds/329 to main July 25, 2025 08:37
@wbrowne wbrowne marked this pull request as ready for review July 25, 2025 08:44
@wbrowne wbrowne requested a review from a team as a code owner July 25, 2025 08:44
@wbrowne wbrowne self-assigned this Jul 25, 2025
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔬 In review
Development

Successfully merging this pull request may close these issues.

3 participants