Conversation
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
| } else { | ||
| lb.Lock() | ||
| lb.ips = ipsToStrings(ips) | ||
| lb.currentIndex = 0 |
There was a problem hiding this comment.
I am not sure about this, as resetting to zero means we will use the first resolved IP more than others.
But I think tests will give the final veredict: Include some tests that prove every ip address gets similar number of requests over many refresh dns.
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
…ip address in the resolved ips Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
…fter roundtrip Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
|
I have added the necessary test @friedrichg. I will handle the log issue in a separate PR. I also would like to refactor the other problem in a separate PR. Let me know if we need to expand the test cases for balancing the load for the resolved ips |
friedrichg
left a comment
friedrichg
left a comment
There was a problem hiding this comment.
This is a first initial attempt. Let's merge it as is and let's work on improvements later
| url, err := url.Parse(upstream.URL) | ||
| if err != nil { | ||
| // TODO: log the error with logrus | ||
| fmt.Println(err) |
There was a problem hiding this comment.
We should exit the function with error. We don't want to continue if the url is not valid
| resolver := DefaultDNSResolver{} | ||
| t := &CustomTransport{ | ||
| Transport: *http.DefaultTransport.(*http.Transport).Clone(), | ||
| lb: newRoundRobinLoadBalancer(url.Hostname(), resolver.LookupIP), |
There was a problem hiding this comment.
newRoundRobinLoadBalancer returns an error too. capture that and if it is nil and return.
We want to abort if we can't resolve the dns to ips.
|
|
||
| reverseProxy := httputil.NewSingleHostReverseProxy(url) | ||
| reverseProxy.Transport = customTransport(component, timeouts) | ||
| reverseProxy.Transport = customTransport(component, upstream) |
There was a problem hiding this comment.
capture the error and exit the function too
| } | ||
|
|
||
| func (m mockDNSResolver) LookupIP(hostname string) ([]net.IP, error) { | ||
| return m.IPs, m.Err |
There was a problem hiding this comment.
Unfortunately this is not how it works. The DNS returns IPs in different order every time.
for example:
First try
$ nslookup cortexmetrics.io
Server: 10.128.4.247
Address: 10.128.4.247#53
Non-authoritative answer:
Name: cortexmetrics.io
Address: 172.67.146.72
Name: cortexmetrics.io
Address: 104.21.73.166
Second try
$ nslookup cortexmetrics.io
Server: 10.128.4.247
Address: 10.128.4.247#53
Non-authoritative answer:
Name: cortexmetrics.io
Address: 104.21.73.166
Name: cortexmetrics.io
Address: 172.67.146.72
I think if you randomize the order here, it can get pretty similar, though
| tolerance float64 | ||
| }{ | ||
| { | ||
| name: "4 IPs, 1000 requests, 1 second refresh interval, 10% tolerance", |
There was a problem hiding this comment.
10% tolerance is probably too much. We will improve it in a follow up PR
2 participants
This PR attempts to implement round robin load balancer