Add half_open_resource_timeout for net http #198
Conversation
max611
force-pushed
the
half_open_http_net
branch
from
719349b to
8eea4bb
Compare
| @@ -42,6 +42,40 @@ def test_semian_identifier | |||
| end | |||
| end | |||
|
|
|||
| def test_changes_timeout_when_half_open_and_configured | |||
There was a problem hiding this comment.
I'm not sure if I'm doing this right, is there a better way of half opening the circuit?
There was a problem hiding this comment.
There's half_open_circuit!
There was a problem hiding this comment.
I think I'll need help with this test, I can't seem to be able to trigger the right thing. I do hit the with_resource_timeout path but ReadTimeout does not raise.
| if half_open? && @half_open_resource_timeout && resource.respond_to?(:with_resource_timeout) | ||
| resource.with_resource_timeout(@half_open_resource_timeout) do | ||
| result = block.call | ||
| result = |
There was a problem hiding this comment.
Small bikeshed, I can bring back the old syntax. I am used to assigning the value directly from the if, not sure which syntax you guys prefer.
There was a problem hiding this comment.
Just for context: The reason this kind of thing might frowned upon is because it causes a 7 line diff that we have to review manually to ensure that a typo size bug isn't added to the library.
| @@ -42,6 +42,40 @@ def test_semian_identifier | |||
| end | |||
| end | |||
|
|
|||
| def test_changes_timeout_when_half_open_and_configured | |||
There was a problem hiding this comment.
There's half_open_circuit!
test/net_http_test.rb
Outdated
|
|
||
| with_semian_configuration(options) do | ||
| with_server do | ||
| Timecop.travel(10) do |
There was a problem hiding this comment.
Can you use an error_timeout variable or index the proc? Makes this easier to understand!
test/net_http_test.rb
Outdated
| end | ||
| end | ||
| open_circuit! | ||
| Semian.resources.values.last.mark_failed(nil) |
There was a problem hiding this comment.
Please don't use these low-level APIs 😬 Can you trigger a real failure here like the other tests? This is very implementation specific.
max611
force-pushed
the
half_open_http_net
branch
6 times, most recently
from
8956e32 to
cd7831e
Compare
max611
force-pushed
the
half_open_http_net
branch
3 times, most recently
from
42dc068 to
81601db
Compare
| @@ -396,6 +432,12 @@ def test_persistent_state_after_server_restart | |||
|
|
|||
| private | |||
|
|
|||
| def half_open_cicuit!(backwards_time_travel = 10) | |||
There was a problem hiding this comment.
@sirupsen I wanted to use the CircuitBreakerHelper but the half_open_circuit! method uses a open_circuit! method with an argument and when I call half_open_circuit! from the net_http_test file, it calls the open_circuit! method inside net_http_test instead of the open_circuit! method inside the helper which raises. To make it work, I would have to change the open_circuit! method name everywhere. What do you think of simply keeping the half_open_cicuit! method in the net_http_test file?
max611
force-pushed
the
half_open_http_net
branch
from
81601db to
bfa0828
Compare
| if half_open? && @half_open_resource_timeout && resource.respond_to?(:with_resource_timeout) | ||
| resource.with_resource_timeout(@half_open_resource_timeout) do | ||
| result = block.call | ||
| result = |
There was a problem hiding this comment.
Just for context: The reason this kind of thing might frowned upon is because it causes a 7 line diff that we have to review manually to ensure that a typo size bug isn't added to the library.
max611
force-pushed
the
half_open_http_net
branch
from
bfa0828 to
0760dc4
Compare
The
half_open_resource_timeoutfeature was introduced here #188 but only for mysql2.This PR adds it for the
net_httpadapter.Net::HTTPlet us assign the timeout by usingread_timeout=.