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.

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

Change how InstanceManager manages instance caching #251

Merged
merged 7 commits into from
Feb 2, 2021
Merged

Change how InstanceManager manages instance caching #251

merged 7 commits into from
Feb 2, 2021

Conversation

santriseus
Copy link
Contributor

@santriseus santriseus commented Jan 16, 2021

This PR changes how InstanceManager creates, caches and disposes of datasource instances.

Fixes #248

Special notes for your reviewer:
Changes:

  1. I added tests which could clarify the issues reported in bug. Without the changes from this PR this tests should fail.
  2. I put the Dispose and code for checking the value existence in the lock section. To optimize it (99.999...% of calls will be just getting the value from cache and returning to the caller after config check) I implemented the double-check locking.
  3. I have replaced simple sync.RWMutex with implementation of named RWMutex, which allow to process instances with different id in parallel without locking.
  4. Since I replaced RWMutex with Named RWMutex we starting to have a situation with parallel read/write call to map with cached instances (for example read instance id#1 and write instance id#2), so I had to replace map with sync.Map

Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Thanks. Codewise looks good. Added some minor comments/suggestions. Still need to test this out manually as well.

backend/instancemgmt/instance_manager.go Outdated Show resolved Hide resolved
backend/instancemgmt/locker.go Show resolved Hide resolved
backend/instancemgmt/locker.go Show resolved Hide resolved
backend/instancemgmt/instance_manager.go Outdated Show resolved Hide resolved
backend/instancemgmt/locker_test.go Show resolved Hide resolved
backend/instancemgmt/locker_test.go Outdated Show resolved Hide resolved
backend/instancemgmt/instance_manager.go Outdated Show resolved Hide resolved
santriseus and others added 4 commits January 21, 2021 14:03
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

See comment

backend/instancemgmt/locker_test.go Show resolved Hide resolved
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr marefr changed the title Change how InstanceManager manages datasource instance caching. Change how InstanceManager manages instance caching Feb 2, 2021
@marefr marefr merged commit 0d4db38 into grafana:master Feb 2, 2021
@marefr
Copy link
Contributor

marefr commented Feb 2, 2021

Thank you for contributing to Grafana!

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.

Possible race condition issues in InstanceManager Get implementation.
3 participants