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

store: Add Groupcache as a cache backend #4818

Merged
merged 26 commits into from Jan 6, 2022

Conversation

akanshat
Copy link
Contributor

@akanshat akanshat commented Oct 30, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Added Groupcache.
  • Implemented Cache interface to groupcache.
  • Added a new package cachekey.

Verification

Existing tests passed.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Seems like you have some unrelated changes here? Could you please rebase on main and don't forget the DCO ;P

@akanshat
Copy link
Contributor Author

Okay, I was hoping you won't see this. 😆

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Looks good, just need to convert to https://github.com/vimeo/galaxycache now + add some e2e tests together with metrics? 😄

@akanshat
Copy link
Contributor Author

akanshat commented Nov 1, 2021

I am planning to make groupcache work properly first, and write a test for it too. Then I want to convert it to galaxycache.

Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

After converting those two methods to have a pointer as the receiver, the metrics appear and I now see the following error when running the e2e tests:

             unexpected error: unable to find metrics [thanos_cache_groupcache_hits_total] with expected values after 50 retries. Last error: <nil>. Last values: [31]

This is probably because Store "refreshed" the view of blocks asynchronously hence increasing that counter (:

@akanshat
Copy link
Contributor Author

@GiedriusS What did you mean by .Get(), I couldn't find any such method here.

@GiedriusS
Copy link
Member

@GiedriusS What did you mean by .Get(), I couldn't find any such method here.

Get() is a method on AtomicInt which is the type of stats.Gets and so on. So, it should be:

	ch <- prometheus.MustNewConstMetric(s.gets, prometheus.CounterValue, float64(stats.Gets.Get()))

And so on, I think.

Signed-off-by: akanshat <akanshat1999@gmail.com>
@akanshat akanshat marked this pull request as ready for review November 17, 2021 12:26
@hanjm
Copy link
Member

hanjm commented Nov 17, 2021

Awesome feature! I am headache in lots of memcache timeout log, groupcache will save me 😄

pkg/cache/groupcache.go Outdated Show resolved Hide resolved
pkg/cache/groupcache.go Show resolved Hide resolved
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
pkg/cache/groupcache.go Show resolved Hide resolved
test/e2e/store_gateway_test.go Outdated Show resolved Hide resolved
test/e2e/store_gateway_test.go Outdated Show resolved Hide resolved
},
)

testutil.Ok(t, store1.WaitSumMetricsWithOptions(e2e.Greater(0), []string{`thanos_cache_groupcache_loads_total`}))
Copy link
Member

Choose a reason for hiding this comment

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

Lets check for the generic metrics we check in memcached test here as well.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
pkg/cache/groupcache.go Outdated Show resolved Hide resolved
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS and others added 12 commits November 17, 2021 18:00
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Adds TTL support to galaxycache as per this pull request:
thanos-community/galaxycache#1

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: akanshat <akanshat1999@gmail.com>
move cache TTLs to a struct
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

It became a big PR in the end but we've done it in small iterations so it's all good 🎉

@@ -1,6 +1,6 @@
# By default we pin to amd64 sha. Use make docker to automatically adjust for arm64 versions.
ARG BASE_DOCKER_SHA="14d68ca3d69fceaa6224250c83d81d935c053fb13594c811038c461194599973"
FROM golang:1.16-alpine3.12 as builder
FROM golang:1.17-alpine3.15 as builder
Copy link
Member

@GiedriusS GiedriusS Jan 6, 2022

Choose a reason for hiding this comment

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

This change also seems unrelated. Probably just needs a rebase since this exact version is used in main. Or you can do git merge origin/main to avoid rewriting the history. I'd prefer that ;P

CHANGELOG.md Show resolved Hide resolved
akanshat and others added 2 commits January 6, 2022 22:56
…cket

Signed-off-by: akanshat <akanshat1999@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS
GiedriusS previously approved these changes Jan 6, 2022
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉
#5037 made this to track the next steps

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Just made some random changes to make the linters happy, I hope you don't mind 👍

@GiedriusS GiedriusS enabled auto-merge (squash) January 6, 2022 20:21
@akanshat
Copy link
Contributor Author

akanshat commented Jan 6, 2022

Thanks!

@GiedriusS
Copy link
Member

Only docs are failing so I'll merge with admin rights 🎉

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

Successfully merging this pull request may close these issues.

None yet

4 participants