Update fallback host logic to reflect comment #162
Conversation
The behavior exhibited by the if statement does not reflect the description in the comment below it: instead of reusing a fallback host if it's been in use for less than a minute, it actually reuses the fallback host only if it was last used more than a minute ago. This was not caught by the test that was added with it, because the test logic is faulty as well: the only way it could fail is if the value of `Time.now` actually decreased over time. This updates the test to assert that the second call completes in less than a second, rather than timing out, and updates the logic to continue to use the current (not first) host if the first host was last called less than a minute ago. Finally, the DNS timeout test context is modified to reset the client and thread local variables before each test, rather than before all of them; otherwise the timeout in the first test case causes the first check in the second test case to fail (since the client has already fallen back to the second host)
4 similar comments
| @@ -399,7 +399,7 @@ def thread_local_hosts(read) | |||
| hosts = Thread.current[thread_hosts_key] | |||
| thread_index_key = read ? "algolia_search_host_index_#{application_id}" : "algolia_host_index_#{application_id}" | |||
| current_host = Thread.current[thread_index_key].to_i # `to_i` to ensure first call is 0 | |||
| if current_host != 0 && hosts[current_host][:last_call].to_i < Time.now.to_i - 60 | |||
| if current_host != 0 && hosts[0][:last_call].to_i > Time.now.to_i - 60 | |||
| # the current_host is not the first one and we've been using it for less than a minute; continue doing so | |||
There was a problem hiding this comment.
Oh wow, the < is wrong, but the current_host is correct.
Scenario:
- do a call
- it targets the host
0and works - do a call
- it targets the host
0and fails - we retry with host
1and storecurrent_host = 1, and works - do a call
- we're on host
1, and it has been less than a minute that we've been using the host1: akahosts[current_host][:last_call].to_i > Time.now.to_i - 60 - it targets host
1, and works - wait 1 min
- do a call
- we're on host
1, and it has been MORE than a minute that we've been using the host1 - return the regular
hostsarray, targets host0and works
|
Oh wow, the Scenario:
|
4 similar comments
|
To be honest, I'm not entirely sure what the desired behavior is, but what I've written more accurately reflects the comment. Using
|
Yes, very confusing :/ But we weren't there yet and because I couldn't commit on your branch; I've opened #163. The test are passing locally, let's see how it goes with Travis.
I'll update the comment 👍 |
The behavior exhibited by the if statement does not reflect the
description in the comment below it: instead of reusing a fallback host if
it's been in use for less than a minute, it actually reuses the fallback
host only if it was last used more than a minute ago.
This was not caught by the test that was added with it, because the test
logic is faulty as well: the only way it could fail is if the value of
Time.nowactually decreased over time.This updates the test to assert that the second call completes in less
than a second, rather than timing out, and updates the logic to continue
to use the current (not first) host if the first host was last called
less than a minute ago.
Finally, the DNS timeout test context is modified to reset the client
and thread local variables before each test, rather than before all of
them; otherwise the timeout in the first test case causes the first
check in the second test case to fail (since the client has already
fallen back to the second host)