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

codestyle: moves cache package to infra #17519

Merged
merged 1 commit into from Jun 13, 2019

Conversation

Projects
None yet
3 participants
@bergquist
Copy link
Contributor

commented Jun 11, 2019

No description provided.

@@ -60,7 +60,7 @@ type HTTPServer struct {
RenderService rendering.Service `inject:""`
Cfg *setting.Cfg `inject:""`
HooksService *hooks.HooksService `inject:""`
CacheService *cache.CacheService `inject:""`
CacheService *localcache.CacheService `inject:""`

This comment has been minimized.

Copy link
@torkelo

torkelo Jun 11, 2019

Member

why local cache? is the interface not a general one that can be backed by remote redis cache?

This comment has been minimized.

Copy link
@bergquist

bergquist Jun 11, 2019

Author Contributor

its not general. I think the usage pattern is different and should be explicit.
I renamed it to make it more clear that's it's local and in memory.

Choosing to cache stuff in memory or serialized remotely is two different actions IMO.

This comment has been minimized.

Copy link
@torkelo

torkelo Jun 11, 2019

Member

is it in memory, as so this is not the same cache that is backed by the database (that has lock issues?) Is this the in memory cache?

This comment has been minimized.

Copy link
@bergquist

bergquist Jun 11, 2019

Author Contributor

Oh, yea. Sorry for not being explicit about that.

This is the in-memory only cache. The cached backed by the database is named remotecache so naming this localcache felt more explicit.

This comment has been minimized.

Copy link
@bergquist

bergquist Jun 12, 2019

Author Contributor

@torkelo anything else to consider before we merge this?

This comment has been minimized.

Copy link
@marefr

marefr Jun 12, 2019

Member

@bergquist may affect enterprise datasource permissions maybe without having looked into the details of the changes

This comment has been minimized.

Copy link
@bergquist

bergquist Jun 12, 2019

Author Contributor

@marefr yes! Ive talked to Leo about it and I will open A pr fixing this in the enterprise so we can merge them at the same time.

@bergquist bergquist requested a review from torkelo Jun 12, 2019

@bergquist bergquist merged commit 6809d2b into grafana:master Jun 13, 2019

2 checks passed

build-branches-and-prs Workflow: build-branches-and-prs
Details
license/cla Contributor License Agreement is signed.
Details

@bergquist bergquist deleted the bergquist:move_cache_to_infra branch Jun 13, 2019

marefr added a commit that referenced this pull request Jun 13, 2019

Fix so that correct cache is provided to di registry (#17566)
This fix prevents Grafana from crashing after being logged in.

Ref #17519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.