Extracted CircuitBreaker states into 3 structures #62

kyewei · 2015-11-10T21:37:12Z

Older closed PR is this: #56
Even older massive PR that we're trying to split is this (probably a bit outdated by this point): #54

Three structures are for holding the @success, @state, and @sliding_window of the CircuitBreaker class.

They are now under Semian::Simple, and future SysV implementation will be under Semian::SysV.

Issue that came up and may/may not have been addressed:

For methods in the classes that explicitly return self as the last line, its so things like #<< don't expose the inner array and leak the implementation, and instead return the appropriate wrapper class. Cascading still works: wrapper_class_instance << 1 << 2
The Atomic prefix was removed since it is misleading, and we probably won't be providing a Mutex or Monitor
it was suggested to rename the enum type from Semian::Simple::Enum to Semian::Simple::State and be specialized for the three possible items: :open, :closed, :half_open. Similarly, also rename from Integer to SuccessCount. I haven't changed it to those names yet, since it requires removing some code and makes some of the code a bit weird.
- Pro: specialized, more simple
- Con: naming would definitely be weird, even if the in-memory version from Simple stores the values directly, the SysV enum type delegates the shared-ness to the SysV integer type, and would need to keep a SysV::SuccessCount type in the SysV::State if we use those names.
A bit looking into the future, but with the SysV versions of the structure needing permissions and a name, Semian::Simple::SlidingWindow.new(max_size: 4) and Semian::SysV::SlidingWindow.new(max_size: 4, permissions: 0660, name: "sample_int") have different interfaces. Ways to solve
- do def initialize(max_size: , **_)
- do def initialize(options = {}) or something of the sort
- not pass in permissions and name at all, but they have to come from somewhere else without leaking the implementation, possible from Semian.register
The enum class has an #increment because it was suggested that it could be used to iterate through the enums. May/may not be needed.
Slightly extraneous: it was previously mentioned that test suite naming is a bit off, for example TestCircuitBreaker is circuit_breaker_test.rb

byroot · 2015-11-10T21:38:15Z

lib/semian/circuit_breaker.rb

-
-    def initialize(exceptions:, success_threshold:, error_threshold:, error_timeout:)
+  class CircuitBreaker #:nodoc:
+    def initialize(name, exceptions:, success_threshold:, error_threshold:, error_timeout:, type_namespace:)


I'm not a fan of type_namespace IMO we should call this an implementation

byroot · 2015-11-10T21:48:30Z

Just a few comments that I believe are minor. Otherwise LGTM.

kyewei · 2015-11-11T15:12:50Z

@sirupsen any thoughts? The one bigger discussion would be this: #62 (diff)

sirupsen · 2015-11-11T15:36:56Z

lib/semian/simple_enum.rb

+
+      def increment(val = 1)
+        @value = @allowed_symbols[(@allowed_symbols.index(@value) + val) % @allowed_symbols.size]
+      end


Is this actually used? The only reason I suggested it before was because you inherited from the integer, since you don't do that anymore, I don't think you actually use this.

It's not used in lib code. It is being tested.

Do you want to go back to a undef

I might need SysV::Enum to inherit from SysV::Integer if byroot's suggestion goes through #62 (diff) because thats the only way it can be done if #value= disappears or goes private

I agree with @byroot's suggestion, which is in line with my comment above that this is not an implementation of an enumerator.

Yeah. That's ok with me. You guys convinced me.

I don't envision how that'd happen. You always go straight from closed to half_open.

No you don't. That's the only transition that doesn't make sense.

Also this never have been a validation....

Too bad I can't just make a create_accessors_and_setters because of english: :closed vs #close. Otherwise I'd just do something like names.each { |name| define_method(name) { @value == name)}.

Do we still want an #increment

sirupsen · 2015-11-11T15:48:18Z

test/simple_sliding_window_test.rb

+      assert_equal(sliding_window_1.last, sliding_window_2.last)
+      assert_equal(sliding_window_1.size, sliding_window_2.size)
+      assert_equal(sliding_window_1.max_size, sliding_window_2.max_size)
+    end


This should just be an array comparison.

You also never use this assertion.

sirupsen · 2015-11-11T15:53:03Z

All my comments are smaller things, looking good Kye. After this iteration, I think it should be ready to go in. Then we'll deploy it to Shopify to make sure all is fine, and start working on the next small PR.

sirupsen · 2015-11-11T15:55:41Z

lib/semian/simple_enum.rb

+module Semian
+  module Simple
+    class Enum #:nodoc:
+      def initialize(symbol_list:)


I wouldn't make this argument named.

Why not? Everything else is named already, like the max_size:

yeah, but it's so key to this class and no default makes sense. It doesn't matter much. Up to you.

kyewei · 2015-11-11T17:12:12Z

Look over again, I suppose?

sirupsen · 2015-11-11T19:05:07Z

LGTM

byroot · 2015-11-11T19:11:16Z

lib/semian/simple_state.rb

+      def value=(val)
+        raise ArgumentError unless @enums.include?(val)
+        @value = val
+      end


This method is unused.

So then, if I remove this test_will_throw_error_when_invalid_symbol_given will be gone as well? Also @enums is not needed either then

Yup. No need to test a method that is not used. Just remove the method and the test althogether.

byroot · 2015-11-11T19:12:26Z

One last concern (unused method). Once it's solved 🐑

kyewei · 2015-11-11T19:24:26Z

Alright, I added @byroot 's suggestion and squashed into one commit. @sirupsen suggested writing an appropriate PR for testing in production before merging it in here.

…tests

sirupsen · 2015-11-12T07:57:18Z

I'd say merge this :)

Extracted CircuitBreaker states into 3 structures

byroot reviewed Nov 10, 2015
View reviewed changes

kyewei force-pushed the separate_cb_structures branch 3 times, most recently from d291681 to 32f68ad Compare November 11, 2015 15:23

sirupsen reviewed Nov 11, 2015
View reviewed changes

kyewei force-pushed the separate_cb_structures branch from 32f68ad to 4269df4 Compare November 11, 2015 15:48

sirupsen reviewed Nov 11, 2015
View reviewed changes

kyewei force-pushed the separate_cb_structures branch from 4269df4 to 7e1e6ff Compare November 11, 2015 15:53

sirupsen reviewed Nov 11, 2015
View reviewed changes

kyewei force-pushed the separate_cb_structures branch from 7e1e6ff to 1cb63ae Compare November 11, 2015 16:51

kyewei force-pushed the separate_cb_structures branch from 1cb63ae to 20335cc Compare November 11, 2015 17:10

kyewei force-pushed the separate_cb_structures branch from 20335cc to ad48ae4 Compare November 11, 2015 18:39

byroot reviewed Nov 11, 2015
View reviewed changes

kyewei force-pushed the separate_cb_structures branch from ad48ae4 to 52b11b9 Compare November 11, 2015 19:22

Extracted CircuitBreaker states into 3 structures, added appropriate …

9b8657f

…tests

kyewei force-pushed the separate_cb_structures branch from 52b11b9 to 9b8657f Compare November 11, 2015 21:19

kyewei added a commit that referenced this pull request Nov 12, 2015

Merge pull request #62 from Shopify/separate_cb_structures

bad6820

Extracted CircuitBreaker states into 3 structures

kyewei merged commit bad6820 into master Nov 12, 2015

kyewei deleted the separate_cb_structures branch November 12, 2015 15:36

kyewei mentioned this pull request Nov 12, 2015

Proposed SysV structures and Mixin functionality #64

Closed

kyewei mentioned this pull request Nov 23, 2015

Entire SysV::Integer code and associated shared libraries #67

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracted CircuitBreaker states into 3 structures #62

Extracted CircuitBreaker states into 3 structures #62

kyewei commented Nov 10, 2015

byroot Nov 10, 2015

kyewei Nov 10, 2015

byroot commented Nov 10, 2015

kyewei commented Nov 11, 2015

sirupsen Nov 11, 2015

kyewei Nov 11, 2015

kyewei Nov 11, 2015

kyewei Nov 11, 2015

sirupsen Nov 11, 2015

sirupsen Nov 11, 2015

byroot Nov 11, 2015

byroot Nov 11, 2015

kyewei Nov 11, 2015

kyewei Nov 11, 2015

sirupsen Nov 11, 2015

sirupsen Nov 11, 2015

sirupsen commented Nov 11, 2015

sirupsen Nov 11, 2015

kyewei Nov 11, 2015

sirupsen Nov 11, 2015

kyewei commented Nov 11, 2015

sirupsen commented Nov 11, 2015

byroot Nov 11, 2015

kyewei Nov 11, 2015

byroot Nov 11, 2015

byroot commented Nov 11, 2015

kyewei commented Nov 11, 2015

sirupsen commented Nov 12, 2015

Extracted CircuitBreaker states into 3 structures #62

Extracted CircuitBreaker states into 3 structures #62

Conversation

kyewei commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Nov 10, 2015

kyewei commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyewei commented Nov 11, 2015

sirupsen commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Nov 11, 2015

kyewei commented Nov 11, 2015

sirupsen commented Nov 12, 2015