add memcached auto-discovery support from thanos #4412
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 |
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.
| @@ -29,6 +30,7 @@ func (cfg *MemcachedClientConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefi | |||
| f.IntVar(&cfg.MaxGetMultiConcurrency, prefix+"max-get-multi-concurrency", 100, "The maximum number of concurrent connections running get operations. If set to 0, concurrency is unlimited.") | |||
| f.IntVar(&cfg.MaxGetMultiBatchSize, prefix+"max-get-multi-batch-size", 0, "The maximum number of keys a single underlying get operation should run. If more keys are specified, internally keys are split into multiple batches and fetched concurrently, honoring the max concurrency. If set to 0, the max batch size is unlimited.") | |||
| f.IntVar(&cfg.MaxItemSize, prefix+"max-item-size", 1024*1024, "The maximum size of an item stored in memcached. Bigger items are not stored. If set to 0, no maximum size is enforced.") | |||
| f.BoolVar(&cfg.AutoDiscovery, prefix+"auto-discovery", false, "Use memcached auto-discovery instead of DNS resolution") | |||
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
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 <[email protected]>
roystchiang
force-pushed
the
upgrade-thanos
branch
from
15704e9 to
269e396
Compare
Signed-off-by: Roy Chiang <[email protected]>
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 <[email protected]> * update documentation Signed-off-by: Roy Chiang <[email protected]> Signed-off-by: Manish Kumar Gupta <[email protected]>
* add memcached auto-discovery support from thanos (cortexproject#4412) Signed-off-by: Roy Chiang <[email protected]> * update documentation Signed-off-by: Roy Chiang <[email protected]> Signed-off-by: Alvin Lin <[email protected]>
Signed-off-by: Roy Chiang [email protected]
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]