reset successes when half opening #192

jpittis · 2018-09-13T17:29:24Z

Seems like in #188 the calls to log_state_transition(:half_open) and @successes.reset were removed.

sirupsen · 2018-09-13T17:44:26Z

These lines didn't change in that PR as far as I can see? What tests are failing? I am not sure what this is doing.

jpittis · 2018-09-13T18:07:48Z

If you look at the current Semian master, there is no code path that resets @successes to zero.

(I'm not sure if this is the right fix / why the tests far failing. Either way, I can't see the code path in the current master that resets the success count.)

pushrax

@sirupsen the bug is subtle and we all missed it in #189. There was previously a call to self.half_open in CircuitBreaker, that was not delegated to @state. #189 effectively changed this call to @state.half_open!, skipping the state transition log and call to @successes.reset. The implicit delegation adds a lot of overhead to understanding what's going on here.

Change makes sense to me but there should be a regression test.

pushrax · 2018-09-13T18:16:22Z

lib/semian/circuit_breaker.rb

@@ -87,7 +87,7 @@ def open

    def half_open
      log_state_transition(:half_open)
-      @state.half_open
+      half_open!


IMO this delegator should just be removed, and State#half_open! can then be renamed back to State#half_open.

sirupsen

I apologize for the initial terse reply, I didn't quite understand the PR and opened it at a time where I didn't have the time to dig in further! I just spent some more time with it this morning. Wonderful find. How did you get here? 👍

At first, I was terrified of the consequences this might have for production. However, on further thought and testing, it seems that the bug here boils down to success_threshold essentially always being the equivalent of 1. I was afraid this would be disastrous, rendering circuits useless, however, then the tests wouldn't have passed.

With that in mind, it makes sense to me no existing test failed since we don't test the success_threshold > 1 case properly between multiple open/close cycles. I wholeheartedly agree with Justin's observation that we need to clean up the delegate. I've done this in a squash of your commit by distinguishing between (1) Am I in the half_open? state, and (2) Am I about to transition to the half_open? state. The former being a state, the latter being a decision the circuit makes. I think this source of confusion is what caused me to introduce the bug in the previous PR. Furthermore, I think that {close,half_open,open} is a tid-bit terse for transitioning between states, so I renamed these methods as well.

@jpittis hope you don't mind, but since this is a fallout from my PR I considered your PR a glorified issue of "hey I found this bug and I think this is a solution" and fixed it here and added a regression test. This test will fail properly with the previous behaviour. There was a subtle bug in the first proposed fix here, since it'd transition to the half_open state every time the condition for half_open? was met. I updated it to only transition once!

Please review @jpittis @pushrax @max611 , then let's get this out this week!

jpittis · 2018-09-17T14:16:07Z

We found this bug because we were re-implementing a Semian circuit breaker in Go for the semian sinulator.

As you say, I meant this PR as an issue + diff.

I didn’t raise a fuss because I came to the same conclusion as you that the bug is relatively harmless.

max611

👍 I love the use of transition_to, makes it easier to read 👀

jpittis

Code LGTM. Looks like we're having issues with Semian test flakyness.

pushrax · 2018-09-17T14:58:19Z

lib/semian/circuit_breaker.rb

@@ -37,33 +37,33 @@ def acquire(resource = nil, &block)
      result
    end

-    def half_open?


This change reverts #189, is that what you wanted?

Yeah, it's fine, I'll use the other helper—the other PR never shipped.

sirupsen · 2018-09-17T16:24:48Z

@max611 would you be able to look into the flaky tests here + ship this to core + bump version + bump Shopify? Might be good to do in parallel when you're waiting for reviews for LRU?

max611 · 2018-09-17T16:27:50Z

@sirupsen Yep will look into it! 👍

pushrax

Tiny nitpicks otherwise LGTM

pushrax · 2018-09-17T23:48:50Z

lib/semian/circuit_breaker.rb

      log_state_transition(:half_open)
-      @state.half_open
+      @state.half_open!


There's a slight inconsistency here now — half_open! gets a bang while open and close do not.

pushrax · 2018-09-17T23:49:52Z

test/circuit_breaker_test.rb

+  def test_open_close_open_cycle
+    resource = Semian.register(:open_close, tickets: 1, exceptions: [SomeError], error_threshold: 2, error_timeout: 5, success_threshold: 2)
+
+    half_open_future_travel = -> { Time.now + resource.circuit_breaker.error_timeout + 1 }


Timecop.travel already computes the absolute time from the delta in this fancy method.

pushrax · 2018-09-17T23:53:33Z

lib/semian/circuit_breaker.rb

      log_state_transition(:open)
      @state.open
    end

-    def half_open
+    def transition_to_half_open
      log_state_transition(:half_open)


I don't think this line is tested.

pushrax · 2018-09-18T18:35:30Z

There were some comments left unaddressed.

max611 · 2018-09-18T18:37:08Z

@pushrax my bad, missed those 🤦‍♂️ I'll create a new PR to address the comments

Addressing comments from #192

pushrax reviewed Sep 13, 2018

View reviewed changes

sirupsen force-pushed the no-success-incr branch from df7af9f to 5d6ac7e Compare September 17, 2018 09:04

sirupsen requested review from sirupsen, pushrax and max611 September 17, 2018 09:10

sirupsen approved these changes Sep 17, 2018

View reviewed changes

sirupsen reviewed Sep 17, 2018

View reviewed changes

max611 approved these changes Sep 17, 2018

View reviewed changes

jpittis commented Sep 17, 2018

View reviewed changes

pushrax reviewed Sep 17, 2018

View reviewed changes

pushrax approved these changes Sep 17, 2018

View reviewed changes

pushrax reviewed Sep 17, 2018

View reviewed changes

reset successes when half opening

5ba188a

max611 force-pushed the no-success-incr branch from 5d6ac7e to 5ba188a Compare September 18, 2018 17:52

max611 merged commit 1d6f931 into master Sep 18, 2018

max611 deleted the no-success-incr branch September 18, 2018 18:34

max611 mentioned this pull request Sep 18, 2018

Addressing comments from #192 #196

Merged

max611 added a commit that referenced this pull request Sep 19, 2018

Merge pull request #196 from Shopify/no_success_follow_up

c56d837

Addressing comments from #192

max611 temporarily deployed to rubygems October 19, 2018 15:07 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset successes when half opening #192

reset successes when half opening #192

jpittis commented Sep 13, 2018

sirupsen commented Sep 13, 2018

jpittis commented Sep 13, 2018 •

edited

Loading

pushrax left a comment •

edited

Loading

pushrax Sep 13, 2018

sirupsen left a comment •

edited

Loading

jpittis commented Sep 17, 2018

max611 left a comment

jpittis left a comment

pushrax Sep 17, 2018

sirupsen Sep 17, 2018

sirupsen commented Sep 17, 2018

max611 commented Sep 17, 2018

pushrax left a comment

pushrax Sep 17, 2018

pushrax Sep 17, 2018

pushrax Sep 17, 2018

pushrax commented Sep 18, 2018

max611 commented Sep 18, 2018

reset successes when half opening #192

reset successes when half opening #192

Conversation

jpittis commented Sep 13, 2018

sirupsen commented Sep 13, 2018

jpittis commented Sep 13, 2018 • edited Loading

pushrax left a comment • edited Loading

Choose a reason for hiding this comment

pushrax Sep 13, 2018

Choose a reason for hiding this comment

sirupsen left a comment • edited Loading

Choose a reason for hiding this comment

jpittis commented Sep 17, 2018

max611 left a comment

Choose a reason for hiding this comment

jpittis left a comment

Choose a reason for hiding this comment

pushrax Sep 17, 2018

Choose a reason for hiding this comment

sirupsen Sep 17, 2018

Choose a reason for hiding this comment

sirupsen commented Sep 17, 2018

max611 commented Sep 17, 2018

pushrax left a comment

Choose a reason for hiding this comment

pushrax Sep 17, 2018

Choose a reason for hiding this comment

pushrax Sep 17, 2018

Choose a reason for hiding this comment

pushrax Sep 17, 2018

Choose a reason for hiding this comment

pushrax commented Sep 18, 2018

max611 commented Sep 18, 2018

jpittis commented Sep 13, 2018 •

edited

Loading

pushrax left a comment •

edited

Loading

sirupsen left a comment •

edited

Loading