Request based on the SRV record of the service #51
Conversation
Current coverage is 96.93% (diff: 100%)
|
|
One of the things that I did not implement is the round robin based on the weight of the record. The |
|
@DennyLoko Thanks for the PR, I appreciate it. I know Can you please help me to understand, this use case is benefits widely? |
|
Hello @jeevatkm! We use an extensive number of Docker containers on our microservices infrastructure, and we use Consul as service discovery mechanism to help us reach each container without the need to have a list of IP addresses of each container of each service. Consul provides a DNS interface for querying the |
|
Thanks for providing real usage scenario. I hope and believe it will be useful for Thanks again for your time and effort. |
There was a problem hiding this comment.
@DennyLoko I have added my recommendations, please have a look and take care.
| domain := strings.TrimRight(addrs[idx].Target, ".") | ||
| path = strings.TrimLeft(path, "/") | ||
|
|
||
| return fmt.Sprintf("%s://%s:%d/%s", DefaultClient.scheme, domain, addrs[idx].Port, path) |
There was a problem hiding this comment.
Please use r.client.scheme instead of DefaultClient.scheme.
| @@ -465,3 +489,15 @@ func (r *Request) fmtBodyString() (body string) { | |||
|
|
|||
| return | |||
| } | |||
|
|
|||
| func (r *Request) selectAddr(addrs []*net.SRV, path string, attempt int) string { | |||
| if addrs == nil { | |||
There was a problem hiding this comment.
Can you please add test case for when addrs is nil and SRV is record is set. So we can ensure request goes through or gives error info to caller.
There was a problem hiding this comment.
Maybe I didn't understand, but I think this test case already cover this situation.
|
@DennyLoko whenever you push your changes, I will merge it. Thanks. |
| @@ -465,3 +489,15 @@ func (r *Request) fmtBodyString() (body string) { | |||
|
|
|||
| return | |||
| } | |||
|
|
|||
| func (r *Request) selectAddr(addrs []*net.SRV, path string, attempt int) string { | |||
| if addrs == nil { | |||
There was a problem hiding this comment.
Maybe I didn't understand, but I think this test case already cover this situation.
|
@DennyLoko no worries, thanks for your response. Changes looks good, can you please squash these commits? |
|
Done 👍 |
|
@DennyLoko Thanks, PR is merged. |
I've made this changes to be possible to do a request based on the SRV record of the service. It is useful when you have a service discovery mechanism, like Consul, so you can pass the information and the system will query the SRV record and to the request based on it.
I do not expect this PR to be merged right away, but to start a discussion on the following topic.
Please, review it and suggest the needed changes.