add memcached auto-discovery support from thanos#4412
add memcached auto-discovery support from thanos#4412alvinlin123 merged 2 commits intocortexproject:masterfrom
Conversation
roystchiang
force-pushed
the
upgrade-thanos
branch
3 times, most recently
from
4cbbf28 to
87ce484
Compare
|
Can you please also review this PR @alolita @Aneurysm9 |
bboreham
left a comment
bboreham
left a comment
There was a problem hiding this comment.
Thanks for this PR.
Since you're upgrading thanos, is this a new feature? If so please link to the upstream details in the PR, maybe also in the CHANGELOG.
Please also check what other changes are brought in by this upgrade (maybe not many, looks like 6 days apart) and state what you found, again in the PR description and CHANGELOG if appropriate.
There was a problem hiding this comment.
Can we get a bit more descriptive than "memcached auto-discovery"?
I Googled that phrase and am not sure I found what you meant.
There was a problem hiding this comment.
I would assume the closest would be: https://docs.aws.amazon.com/AmazonElastiCache/latest/mem-ug/AutoDiscovery.HowAutoDiscoveryWorks.html
There was a problem hiding this comment.
There was a problem hiding this comment.
I have updated the PR description to include the thanos PR link. Also added 2 links for explaining what auto-discovery means for memcached.
There are no other changes brought by upgrading Thanos in this PR.
Please let me know if you have other concerns.
There was a problem hiding this comment.
@bboreham do you have anymore concern on this PR?
There was a problem hiding this comment.
The Changelog entry is circular and the Thanos link doesn't explain auto-discovery.
I think you're saying it is an ElasticCache feature that is also supported by Google Cloud; the Cortex docs should link to one or both of those if there is nothing more neutral.
roystchiang
force-pushed
the
upgrade-thanos
branch
from
87ce484 to
3821a29
Compare
roystchiang
force-pushed
the
upgrade-thanos
branch
from
3821a29 to
c41ebcd
Compare
alvinlin123
left a comment
alvinlin123
left a comment
There was a problem hiding this comment.
Since the auto_discovery feature here is Cloud provider specific, I think we should add some document like
https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery
or maybe in
roystchiang
force-pushed
the
upgrade-thanos
branch
from
c41ebcd to
3a48b0d
Compare
roystchiang
force-pushed
the
upgrade-thanos
branch
from
3a48b0d to
0f57fb9
Compare
roystchiang
force-pushed
the
upgrade-thanos
branch
from
0f57fb9 to
15704e9
Compare
Signed-off-by: Roy Chiang <roychi@amazon.com>
roystchiang
force-pushed
the
upgrade-thanos
branch
from
15704e9 to
269e396
Compare
Signed-off-by: Roy Chiang <roychi@amazon.com>
roystchiang
force-pushed
the
upgrade-thanos
branch
from
774c10d to
2d05ccf
Compare
* add memcached auto-discovery support from thanos (cortexproject#4412) Signed-off-by: Roy Chiang <roychi@amazon.com> * update documentation Signed-off-by: Roy Chiang <roychi@amazon.com> Signed-off-by: Manish Kumar Gupta <manishkg@microsoft.com>
* add memcached auto-discovery support from thanos (cortexproject#4412) Signed-off-by: Roy Chiang <roychi@amazon.com> * update documentation Signed-off-by: Roy Chiang <roychi@amazon.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
5 participants
Signed-off-by: Roy Chiang roychi@amazon.com
What this PR does:
Updates Thanos to a version that includes memcached auto-discovery support, introduced in thanos-io/thanos#4487.
Auto-discovery is a way for clients to automatically discover nodes belonging to a memcached cluster. It is different from DNS discovery because the node resolution is not done on the DNS level, but rather through talking to memcached.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]