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.

Sign up for GitHub

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

Jump to bottom

query: add memcached autodiscovery support #4487

Merged
merged 7 commits into from
Aug 3, 2021
Merged

query: add memcached autodiscovery support #4487

merged 7 commits into from
Aug 3, 2021

Conversation

roystchiang
Copy link
Contributor

@roystchiang roystchiang commented Jul 26, 2021 •
edited
Loading

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

Changes

  • Add a flag to enable auto-discovery for memcached
  • Add a new provider/resolver for auto-discovery

Verification

  • Integrated into cortex, and verified that auto-discovery is able to find scaled up nodes without having to restart pods
  • Tested with AWS ElastiCache Auto Discovery, but should work with Google Cloud too, since the protocol seem the same

I also updated the memcached config documentation so that we can close #3879

@roystchiang roystchiang force-pushed the autodiscovery branch 3 times, most recently from 3644367 to f86af27 Compare July 26, 2021 19:50
GiedriusS
GiedriusS reviewed Jul 26, 2021
View reviewed changes
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.

Please rebase on latest main, that should fix CircleCI

@roystchiang
add memcached autodiscovery support
af820e7 
Signed-off-by: Roy Chiang <roychi@amazon.com>
@roystchiang
pass logger to provider struct
0e8cbbc 
Signed-off-by: Roy Chiang <roychi@amazon.com>
@GiedriusS
Copy link
Member

@roystchiang could you please fix the linter messages? 🤗

@roystchiang
fix linter issues
933b325 
Signed-off-by: Roy Chiang <roychi@amazon.com>
@roystchiang
Copy link
Contributor Author

oops. I've fixed the linter messages now.

@roystchiang
remove copypasted comment
f3b2dfb 
Signed-off-by: Roy Chiang <roychi@amazon.com>
GiedriusS
GiedriusS reviewed Jul 28, 2021
View reviewed changes
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.

A few nits but overall good work 👍

pkg/discovery/memcache/provider.go Show resolved Hide resolved
pkg/discovery/memcache/resolver.go Outdated Show resolved Hide resolved
pkg/cacheutil/memcached_client.go Outdated Show resolved Hide resolved
@roystchiang
address comments
e80b1f3 
Signed-off-by: Roy Chiang <roychi@amazon.com>
GiedriusS
GiedriusS previously approved these changes Aug 2, 2021
View reviewed changes
docs/components/store.md Outdated Show resolved Hide resolved
@GiedriusS
Copy link
Member

@roystchiang please add the DCO and let's merge 🤗

@GiedriusS
Copy link
Member

@roystchiang make docs now fails :/

@roystchiang
Copy link
Contributor Author

hahahahahaha. yeah I'm on it. sorry about that

@roystchiang @GiedriusS
Update docs/components/store.md
22ed612 
Co-authored-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
GiedriusS
GiedriusS previously approved these changes Aug 2, 2021
View reviewed changes
pkg/discovery/memcache/provider.go Show resolved Hide resolved
@roystchiang
add lock/unlock for autodiscovery provider
6c852d0 
Signed-off-by: Roy Chiang <roychi@amazon.com>
@GiedriusS GiedriusS merged commit baea4ce into thanos-io:main Aug 3, 2021
@roystchiang roystchiang mentioned this pull request Aug 23, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@GiedriusS GiedriusS GiedriusS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Give guidance that memcached should not be configured on loadbalancer level
2 participants
@roystchiang @GiedriusS