-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove query of ENV on each circuit breaker acquire. #427
Conversation
c324d39
to
87ab933
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I see no documentation in the README about these environment variables. Might be worth including that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback is mostly questions. I haven't looked too deeply into the APIs in this gem.
Also TIL ENV[]
is slower than ENV.key?
Warming up --------------------------------------
[] 511.958k i/100ms
.key? 928.543k i/100ms
Calculating -------------------------------------
[] 5.104M (± 0.6%) i/s - 25.598M in 5.015834s
.key? 9.333M (± 0.4%) i/s - 47.356M in 5.073909s
Comparison:
.key?: 9333316.7 i/s
[]: 5103582.6 i/s - 1.83x (± 0.00) slower
Benchmark code
# frozen_string_literal: true
require "benchmark/ips"
Benchmark.ips do |x|
x.report("[]") { ENV["HOME"] }
x.report(".key?") { ENV.key?("HOME") }
x.compare!
end
Hi folks. If you have time, pls review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly clarity in docs but also a compatibility question
test/circuit_breaker_test.rb
Outdated
def test_semian_wide_env_var_disables_circuit_breaker | ||
def test_semian_wide_env_var_does_not_create_resource | ||
ENV["SEMIAN_DISABLED"] = "1" | ||
open_circuit! | ||
|
||
assert_circuit_closed | ||
resource = Semian.register( | ||
:global_env_var_disables_circuit_breaker, | ||
tickets: 1, | ||
error_threshold: 2, | ||
error_timeout: 5, | ||
success_threshold: 1, | ||
) | ||
|
||
assert_nil(resource) | ||
ensure | ||
ENV.delete("SEMIAN_DISABLED") | ||
end | ||
|
||
def test_semian_wide_env_disables_totaly_even_with_explicit_circuit_breaker | ||
ENV["SEMIAN_DISABLED"] = "1" | ||
|
||
resource = Semian.register( | ||
:global_env_var_disables_circuit_breaker, | ||
bulkhead: true, | ||
tickets: 1, | ||
circuit_breaker: true, | ||
error_threshold: 2, | ||
error_timeout: 5, | ||
success_threshold: 1, | ||
) | ||
|
||
assert_nil(resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we always returned a non-nil value from .register
before (or raised an exception). Is this method exposed to apps using Semian?
If so, and I'm reading it correctly, this may not be backwards-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly. The problem that before it would not be raise, but NOOP. Because it created always resource objects, and skip logic during on acquire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example when it should raise:
Semian.register(:raise, {bulkhead: false, circuit_breaker: false})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, but the contract of this method has still changed in this PR, if I understand correctly. If the return value of Semian.register
was ever intended to be meaningful outside the gem, apps could now be receiving nil
values that they weren't before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true. I am going to stress it in changelog and also in gemspec post install notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found that Semian.register
requires return some resource.
Using NetHTTP adapter as example, I found that it raises error if semian is disabled.
Semian resource for network is initialized on acquire if it is missing.
As workaround I return UnprotectedResource
if SEMIAN_DISABLED
exists.
2efef40
to
dd417b2
Compare
… method. Move logic of disabled? for Circuit breaker to Initalization part. Circtuit breaker should have any knowledge of ENV or configurations. Update using of SEMIAN_DISABLED to disable Semian without creating any resources. It would allow only check only once if CircuitBreaker exists in resource acquire. If there is any of ENV `SEMIAN_CIRCUIT_BREAKER_DISABLED` or `SEMIAN_DISABLED` exists, then it would disable Circuit breaker initialization.
Found that there is checking on ENV variable on each acquire invoking method.
Move logic of disabled? for Circuit breaker to Initialization part.
Circtuit breaker should not have any knowledge of ENV or configurations.
Update using of SEMIAN_DISABLED to disable Semian without creating any resources.
It would allow only check only once if CircuitBreaker exists in resource acquire.
If there is any of ENV
SEMIAN_CIRCUIT_BREAKER_DISABLED
orSEMIAN_DISABLED
exists,then it would disable Circuit breaker initialization.
Environment variable
SEMIAN_CIRCUIT_BREAKER_DISABLED
has higher priority over explicit optioncircuit_breaker: true
.Changes: When environment variable
SEMIAN_DISABLED
is set, then it would returnUnprotectedResource
, instead ofProtectedResource
.