Use more of the cortex frontend module to deduplicate some code and add the memcached backend by krasi-georgiev · Pull Request #3114 · thanos-io/thanos

krasi-georgiev · 2020-09-02T12:29:58Z

taking over for Bens PR :#3032

Signed-off-by: Krasi Georgiev 8903888+krasi-georgiev@users.noreply.github.com

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

yeya24

Though this pr is still a draft, I just take an initial look. Hope you don't mind.

We are reusing Cortex config directly here. My idea is:

We can reuse the Cortex cache config. But it is not very good to reuse its Limits and QueryRange config, they contain some unnecessary fields for Thanos so what about using our own? It wouldn't be too much code to maintain, just some structs/type conversion between Cortex and Thanos.

cmd/thanos/query-frontend.go

docs/components/query-frontend.md

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2020-09-02T13:34:59Z

Though this pr is still a draft, I just take an initial look. Hope you don't mind.

We are reusing Cortex config directly here. My idea is:

We can reuse the Cortex cache config. But it is not very good to reuse its Limits and QueryRange config, they contain some unnecessary fields for Thanos so what about using our own? It wouldn't be too much code to maintain, just some structs/type conversion between Cortex and Thanos.

Yep I removed the Limits we can stil use the constructor though.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

docs/components/query-frontend.md

pkg/queryfrontend/roundtrip.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

kakkoyun · 2020-09-07T12:00:15Z

@krasi-georgiev I believe this will add support for memcached as a cache backend. Could you please update the title and changelog accordingly?

CHANGELOG.md

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2020-09-07T13:25:32Z

@krasi-georgiev I believe this will add support for memcached as a cache backend. Could you please update the title and changelog accordingly?

updated thanks

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

docs/components/query-frontend.md

pkg/queryfrontend/config.go

cmd/thanos/query-frontend.go

pkg/queryfrontend/roundtrip.go

yeya24

Thanks for all the work! 🥇

From my point of view, it is all good, just need to update the tests. Can I have some other 👀 to review this pr @kakkoyun @squat? Also some flags name and configs are changed, it would be good to add these changes to the changelog.

The Cortex cache config is a little bit complex, it would be good to add a Cortex doc link https://cortexmetrics.io/docs/configuration/configuration-file/. What do you think?

cmd/thanos/query-frontend.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2020-09-14T19:48:04Z

refactored to be consistent with the other Thanos cache components

please take another look @kakkoyun @bwplotka @yeya24

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

kakkoyun

LGTM. Let's what others also think.

bwplotka

Awesome, minor nits only otherwise LGTM, thanks!

cmd/thanos/query-frontend.go

bwplotka · 2020-09-15T20:59:07Z

pkg/queryfrontend/config.go

+					Addresses:      strings.Join(config.Memcached.Addresses, ","),
+					UpdateInterval: config.Memcached.DNSProviderUpdateInterval,
+				},
+				Background: cortexcache.BackgroundConfig{


BackgroundConfig 👀

Anything looks strange here?

test/e2e/query_frontend_test.go

test/e2e/query_test.go

yeya24

Some very small nits, overall LGTM 🚀

pkg/queryfrontend/config.go

pkg/queryfrontend/roundtrip.go

pkg/queryfrontend/config.go

scripts/cfggen/main.go

test/e2e/e2ethanos/services.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

yeya24

LGTM!

Use more of the cortex frontend module to deduplicate a lot code

200cd66

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

yeya24 requested changes Sep 2, 2020

View reviewed changes

tidy up the configs

ebdaec1

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

review nits

779cf0c

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

yeya24 reviewed Sep 2, 2020

View reviewed changes

docs/components/query-frontend.md Show resolved Hide resolved

docs/components/query-frontend.md Outdated Show resolved Hide resolved

pkg/queryfrontend/roundtrip.go Show resolved Hide resolved

krasi-georgiev added 3 commits September 2, 2020 21:15

use own structs

d93f36c

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

use deducated struct for query range and add validation

efe9fe5

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

remove the memcache tests as these are already tested in cortex

b010dc3

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

kakkoyun mentioned this pull request Sep 7, 2020

Support memcached for query frontend #3032

Closed

2 tasks

kakkoyun reviewed Sep 7, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

krasi-georgiev changed the title ~~[WIP] Use more of the cortex frontend module to deduplicate a lot code~~ [WIP] Use more of the cortex frontend module to deduplicate a lot code and add all supported backends by cortex Sep 7, 2020

fixed the tests

0882188

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev added 3 commits September 7, 2020 16:27

nit

3ed8683

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into memcached-query

99bc2b4

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

go mod tidy

c8b4140

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the memcached-query branch from 9cc37c8 to c8b4140 Compare September 7, 2020 13:47

krasi-georgiev marked this pull request as ready for review September 7, 2020 14:26

Merge remote-tracking branch 'origin/master' into memcached-query

51bcf43

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev changed the title ~~[WIP] Use more of the cortex frontend module to deduplicate a lot code and add all supported backends by cortex~~ Use more of the cortex frontend module to deduplicate a lot code and add all supported backends by cortex Sep 7, 2020

generate docs

a2ff3f6

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

yeya24 reviewed Sep 7, 2020

View reviewed changes

krasi-georgiev force-pushed the memcached-query branch 2 times, most recently from 0a9ed13 to b0bef06 Compare September 7, 2020 15:34

yeya24 reviewed Sep 7, 2020

View reviewed changes

cmd/thanos/query-frontend.go Outdated Show resolved Hide resolved

nits

f152db6

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the memcached-query branch from b0bef06 to f152db6 Compare September 7, 2020 15:45

rearange imports, link to cortex docs

19b2c7f

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev added 8 commits September 14, 2020 12:37

Merge remote-tracking branch 'origin/master' into memcached-query

521491b

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

gen docs after changes

1d8a571

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

attempt to use the thanos memcache configs

f96a44b

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

refactored

8d75db0

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into memcached-query

fbc92e8

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

fix the tests

d21fe08

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

fix invalid pointers

bb1471f

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

nits

5c4335c

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev changed the title ~~Use more of the cortex frontend module to deduplicate a lot code and add all supported backends by cortex~~ Use more of the cortex frontend module to deduplicate a lot code and add the memcached backend Sep 14, 2020

krasi-georgiev changed the title ~~Use more of the cortex frontend module to deduplicate a lot code and add the memcached backend~~ Use more of the cortex frontend module to deduplicate some code and add the memcached backend Sep 14, 2020

fix e2e tests

026794d

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the memcached-query branch from 3b18b76 to 026794d Compare September 14, 2020 19:46

add warning for the MaxItemSize

f172f9d

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the memcached-query branch from 83bdf2c to f172f9d Compare September 14, 2020 20:12

kakkoyun approved these changes Sep 15, 2020

View reviewed changes

bwplotka approved these changes Sep 15, 2020

View reviewed changes

yeya24 reviewed Sep 16, 2020

View reviewed changes

krasi-georgiev added 4 commits September 17, 2020 14:01

review comments

ba9b6e5

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into memcached-query

422bd42

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

gen docs

40b2fed

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

links

c41ba79

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the memcached-query branch from fd595a5 to c41ba79 Compare September 17, 2020 12:04

yeya24 approved these changes Sep 18, 2020

View reviewed changes

kakkoyun merged commit 17343c1 into thanos-io:master Sep 18, 2020

krasi-georgiev deleted the memcached-query branch September 18, 2020 14:54

yeya24 mentioned this pull request Sep 28, 2020

Cut 0.16.0-rc.0 #3247

Merged

This was referenced Jan 13, 2021

Support v0.16.0 - Rename parameter query-frontend.log_queries_longer_than to query-frontend.log-queries-longer-than syberalexis/puppet-thanos#29

Merged

[FEATURE] Support for Thanos >= v0.16.0 syberalexis/puppet-thanos#30

Closed

craigfurman mentioned this pull request Mar 8, 2021

Supporting max item size for the memcached client (query-frontend) #3895

Closed

Conversation

krasi-georgiev commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krasi-georgiev commented Sep 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kakkoyun commented Sep 7, 2020

Uh oh!

Uh oh!

krasi-georgiev commented Sep 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yeya24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krasi-georgiev commented Sep 14, 2020

Uh oh!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krasi-georgiev commented Sep 2, 2020 •

edited

Loading

yeya24 left a comment •

edited

Loading