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 Memcached Index Cache mixin #106

Merged
merged 7 commits into from
Apr 8, 2020

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Apr 6, 2020

This PR adds withMemcachedIndexCache to Thanos Store to use memcached as index cache.
Enables posting compression.

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

Changes

  • Adds withMemcachedIndexCache mixin.
  • Enables posting compression.
  • Added flags from Thanos Store:
    • --experimental.enable-index-cache-postings-compression
    • --index-cache.config=${CONFIG}'

Verification

  • make generate

@brancz
Copy link
Member

brancz commented Apr 6, 2020

DCO missing, otherwise looks really awesome!

@kakkoyun kakkoyun changed the title Add Thanos Store memcached mixin WIP: Add Thanos Store memcached mixin Apr 6, 2020
@kakkoyun kakkoyun changed the title WIP: Add Thanos Store memcached mixin Add Thanos Store memcached mixin Apr 6, 2020
@kakkoyun kakkoyun changed the title Add Thanos Store memcached mixin WIP: Add Thanos Store memcached mixin Apr 6, 2020
@kakkoyun kakkoyun force-pushed the memcached_index_cache branch 2 times, most recently from 67c23c9 to aebb282 Compare April 6, 2020 15:51
@kakkoyun kakkoyun changed the title WIP: Add Thanos Store memcached mixin Add Thanos Store memcached mixin Apr 6, 2020
all.jsonnet Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
all.jsonnet Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
config+:: {
memcached+: {
addresses: error 'must provide memcached addresses',
timeout: '0s',
Copy link
Member Author

Choose a reason for hiding this comment

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

@pracucci @pstibrany I'm trying to come up with sane default for memcached, could point me to the right direction? What works for you best?

Copy link

Choose a reason for hiding this comment

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

It really depends on the scale you wanna address. I still think the defaults I've set into Thanos are OK for a small/medium scale:
https://github.com/thanos-io/thanos/blob/1a2edf19475b854181e5abaa4b6f1c9caf2a1547/pkg/cacheutil/memcached_client.go#L35-L44

If you wanna be a bit more aggressive you may consider:

timeout: '2s',
maxIdleConnections: 1000,
maxAsyncConcurrency: 100,
maxAsyncBufferSize: 100000,
maxGetMultiConcurrency: 900,
maxGetMultiBatchSize: 1000,

I've also seen you set dnsProviderUpdateInterval: '3s'. I'm not sure there's a real value of being that aggressive here. The default 10s could be enough (but maybe you find a good reason to lower it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for the response, this was really helpful.

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun requested a review from metalmatze April 8, 2020 13:44
@kakkoyun
Copy link
Member Author

kakkoyun commented Apr 8, 2020

This is in decent shape to merge, could you have another look @metalmatze?

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, some suggestions only.

all.jsonnet Outdated Show resolved Hide resolved
all.jsonnet Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

nice!

@kakkoyun kakkoyun merged commit bc2e452 into thanos-io:master Apr 8, 2020
@kakkoyun kakkoyun deleted the memcached_index_cache branch April 8, 2020 18:25
@kakkoyun kakkoyun changed the title Add Thanos Store memcached mixin store: Add Memcached Index Cache mixin May 15, 2020
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.

None yet

6 participants