Changed next_endpoint method from random to round robin
#124
Conversation
| fn next_endpoint(&self, already_used_endpoints: &[String]) -> String { | ||
| let mut endpoints = self.endpoints.clone(); | ||
|
|
||
| // if all endpoints are already used, return random endpoint | ||
| if endpoints.len() == already_used_endpoints.len() { | ||
| return already_used_endpoints | ||
| [rand::thread_rng().gen_range(0..already_used_endpoints.len())] | ||
| .to_string(); | ||
| } | ||
|
|
||
| // round robin endpoints that are not already used | ||
| let mut endpoint = endpoints.remove(0); | ||
| while already_used_endpoints.contains(&endpoint) { | ||
| endpoint = endpoints.remove(0); | ||
| } | ||
| endpoint |
There was a problem hiding this comment.
Usual algorithm for this is to have an atomic counter, always add 1 to it and you do a x % N to get the next endpoint.
Seems you are adding lot of complexity with the already_used_endpoints, is it really required?
There was a problem hiding this comment.
Ok so it's kind of a different thing than round robin, it's more a random + blocked endpoints. I suggest you use a HashSet to hold your already used endpoints. You than "diff" the blocked set with the full set which gives you the unused set and do a random pick on the "free" set.
The overall code will read much better. It will also remove the need for cloning the endpoints initially since you will diff two sets in read-only to get a new one which you will use to pick an element.
Also, it's probably better to remove the random pick and move to a real round robin also. But it complicates a bit when doing that use next not visited.
Normally I would have suggested you initially to just to a real round robin without the "used" list first. Usually it's enough, a random on small list usually yields bad results on retry since you often go back on the same value often.
| log::info!( | ||
| "retrying request in {} second(s), at attempt {}, attempts left {}", | ||
| log::warn!( | ||
| "{{ \"endpoint\": \"{}\", \"path\": \"{}\", \"status\": \"{}\", \"retry_in\": {}, \"attempts\": {}, \"attempts_left\": {} }}", |
There was a problem hiding this comment.
Why emitting JSON format now? Is it for some log extraction?
| continue; | ||
| } | ||
|
|
||
| // If all endpoints are used, wait then retry with random endpoint |
There was a problem hiding this comment.
This comment is misleading as if retry is 3 but your have 5 endpoints, then it will not have exhausted all tries.
next_endpointis now doing a round robin rotation to provide new endpoint