-
Notifications
You must be signed in to change notification settings - Fork 795
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
Support Redis dns discovery #4590
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming I understand what you did, which looks like a DNS lookup, I am tempted to suggest re-using the 'address provider' mechanism we borrow from Thanos in other caches which allows dns+
and dnssrv+
prefixes.
Maybe I'm missing something specific to Redis?
pkg/chunk/cache/redis_client.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
addrs, err := net.LookupHost(host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like a straightforward DNS lookup - why do you call it "Cluster configuration" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a term that AWS uses (only?), the cluster configuration is an A record which resolves all node addresses in a cluster, I used cluster configuration
to hint that it could be single DNS record pointing to a cluster and reuse the AWS term.
But let me know if you have a suggestion for naming a more generic DNS record pointing to a redis cluster nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi siavashs, first of all, thank you for the contribution!
There is actually a service discover through DNS mechanism for memcached called; here is the doc. For Redis I think we should be consistent and use similar way to configure DNS look up.
Hi @alvinlin123 Thanks for pointing that out. Yes, I think that's a better solution. |
I've updated the PR to add support for dns discovery as suggested. |
@siavashs Sorry for the delay, but I just remembered this pending PR. Would you kindly fix the lint error? Else the change looks good to me |
} | ||
resolved, err := resolver.Resolve(ctx, name, dns.QType(qtype)) | ||
if err != nil { | ||
level.Error(logger).Log("msg", "failed to resolve redis address", "addr", addr, "err", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistically, is it better to log here rather than returning the error?
What happens if all the addresses result in error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, is it better to return an error on first address resolve failure or return only if all addresses fail to resolve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the second option to return an error if no addresses are resolved.
The go-redis options requires multiple redis addresses for a cluster. In some scenarios like AWS ElastiCache, a single configuration endpoint is used to resolve all cluster nodes. This change adds experimental support for redis dns discovery. This is a similar implementation to memcached ds but with just a resolver. Signed-off-by: Siavash Safi <siavash.safi@gmail.com>
The CI tests have failed because of a timeout but I can't rerun them. |
This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions. |
What this PR does:
The go-redis options requires multiple redis addresses for a cluster.
In some scenarios like AWS ElastiCache, a single configuration endpoint is used to resolve all cluster nodes.
This change adds experimental support for redis dns discovery.
This is a similar implementation to memcached ds but with just a resolver.
Signed-off-by: Siavash Safi siavash.safi@gmail.com
Which issue(s) this PR fixes:
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]