Subscriptions redesigned #665

mpraglowski · 2019-08-16T14:10:58Z

No description provided.

Introducing 2 new classes, Subscription & GlobalSubscription. When new subscription is added an instance of one of tis classes will be a result of add method instead of unsubscribe proc - this is breaking change. Unsubscribe now could be handled by calling on returned subscription object an unsubscribe method. Also dispatcher is affected here. Instead of subscriber (object or class) it will receive as a first argument an subscription object. If subscriber is a class a subscription object is now responsible for instantialize new subscriber object (that have been responsibility of a dispatcher).

Remove thread specific subscriptions, replace with more generic LocalSubscriptions & GlobalSubscription with concurent subscription store. Allow to pass local & global store classes to Subscriptions.

…of subscriber

…mentation detail

…re & thread subscription (temporary subscriptions) stores only.

joelvh · 2019-08-16T14:19:20Z

I like this direction. What issues are you addressing with this?

mpraglowski · 2019-08-16T14:58:31Z

I've started with instrumentation of subscriptions. In current codebase it would be easy to just add instrumentation when you add subscription but unsubscribe is not that easy. Later I've have other ideas, so current implementation aims at:

allow to instrument subscribe & unsubscribe
define Subscription object to encapsulate all operations on subscription (no more lambda to unsubscribe, just find your subscription & call unsubscribe method)
easy access to information about subscriptions
allow to provide your own subscription store (will allow to implement persisted subscriptions)

joelvh · 2019-08-16T15:38:25Z

Ok thanks!

…red is not executed during mutation tests

It will allow better control on how thread store is created.

joelvh · 2019-08-16T17:58:41Z

We created a class-based DSL to make it easy to create event handlers for read models. We have an async option that builds a Sidekiq worker class with the given proc. (It uses a naming convention for the Sidekiq worker process to resolve the proper worker class.)

I wonder if this new object model might make it possible to enable some async convention as well? The main challenge is to identify the correct proc between the web and worker processes.

Thoughts?

Example DSL we use:

class UserReadModelDenormalizer < EventHandler
  on Events::UserRegistered, async: true do |event|
    Mailers::RegistrationConfirmation.deliver(event.data[:email])
  end
end

This would create a class such as UserReadModelDenormalizer::Worker_HandleUserRegistration that gets enqueued and picked up by Sidekiq.

joelvh · 2019-08-16T17:59:37Z

I should note, to make sure naming is consistent, I think we actually explicitly specify a name for the worker currently using async: 'HandleUserRegistration' which creates UserReadModelDenormalizer::Worker_HandleUserRegistration

joelvh · 2019-08-16T18:23:22Z

cc @paneq @pawelpacana

joelvh · 2019-08-16T18:25:42Z

@paneq @pawelpacana I saw something about "contrib" flying by a little while ago. I have developed various DSLs to organize things in our projects and wonder if some of that might be good to go in contrib? It's mostly base classes with DSLs similar to what I commented with here, to define handlers and also commands and some other things. Would that be a fit for contrib?

joelvh · 2019-08-16T18:26:35Z

One more thought - would RailsEventStore ever use things from "contrib", or is contrib a superset of Rails?

joelvh · 2019-08-16T18:33:35Z

Ah, I was thinking of #641

mpraglowski · 2019-08-17T08:16:25Z

@joelvh

I saw something about "contrib" flying by a little while ago.

Contrib is free for all to experiment - not released yet, don't know if it should be as a single gem or each idea as a separate gem under common namespace - to be decided yet. Best ideas could be incorporated into RES.

I have developed various DSLs to organize things in our projects and wonder if some of that might be good to go in contrib?

Sure! It will be good to have it there. Fee free to create new folder and gem inside contrib - similar to my transformations gem https://github.com/RailsEventStore/rails_event_store/tree/master/contrib/transformations

FrozenError has been introduced in Ruby 2.5

mostlyobvious · 2019-09-17T16:03:53Z

ruby_event_store/lib/ruby_event_store/global_subscription.rb

+
+    def unsubscribe
+      @store.delete(self)
+    end


with #call aliased to #unsubscribe this would not be a breaking change, but #call is already taken...

mostlyobvious · 2019-09-17T16:06:25Z

ruby_event_store/lib/ruby_event_store/global_subscription.rb

+    #
+    # @param other [GlobalSubscription, Object] object to compare
+    #
+    # Event equality ignores metadata!


Event... copy paste?

mostlyobvious · 2019-09-17T16:19:01Z

ruby_event_store/lib/ruby_event_store/subscription.rb

+      [
+        self.class,
+        event_types,
+        subscriber.object_id


why object_id?

mostlyobvious · 2019-09-17T16:23:33Z

ruby_event_store/lib/ruby_event_store/subscriptions.rb

+
+    class GlobalStore
+      def initialize
+        @subscriptions = []


Wouldn't it make more sense to have a Set here?

mostlyobvious · 2019-09-17T16:24:59Z

ruby_event_store/lib/ruby_event_store/subscriptions.rb

@@ -33,6 +33,42 @@ def all_for(event_type)
    private
    attr_reader :local, :global, :thread

+    class Store
+      def initialize
+        @subscriptions = Hash.new {|hsh, key| hsh[key] = [] }


If we don't intend to store same subscription multiple times, I'd rather see Set here

mostlyobvious · 2019-09-17T16:33:38Z

ruby_event_store/lib/ruby_event_store/subscriptions.rb

-        @store.all
+      def build_store(klass)
+        var = Concurrent::ThreadLocalVar.new(klass.new)
+        var.value = klass.new


Is var.value= call needed right after constructing ThreadLocalVar?

v = ThreadLocalVar.new(14) v.value #=> 14 v.value = 2 v.value #=> 2

mostlyobvious · 2019-09-17T16:43:13Z

ruby_event_store/lib/ruby_event_store/global_subscription.rb

@@ -1,17 +1,17 @@
 module RubyEventStore
  class GlobalSubscription
-    def initialize(subscriber, store)
+    def initialize(subscriber, store: nil)


I don't yet see the purpose, checking next commits.

mostlyobvious · 2019-09-17T16:45:10Z

ruby_event_store/lib/ruby_event_store/in_memory_subscriptions_store.rb

+      @subscriptions = Hash.new {|hsh, key| hsh[key] = [] }
+    end
+
+    def add(subscription, type = nil)


https://blog.arkency.com/using-singleton-objects-as-default-arguments/

mostlyobvious · 2019-09-17T16:47:07Z

ruby_event_store/lib/ruby_event_store/in_memory_subscriptions_store.rb

+    end
+
+    def add(subscription, type = nil)
+      @subscriptions[type.to_s] << subscription


Why to_s? Aren't we expecting Event#type after all?

mostlyobvious · 2019-09-17T16:51:54Z

ruby_event_store/lib/ruby_event_store/in_memory_subscriptions_store.rb

@@ -4,22 +4,18 @@ def initialize
      @subscriptions = Hash.new {|hsh, key| hsh[key] = [] }
    end

-    def add(subscription, type = nil)
+    def add(subscription, type = GLOBAL_SUBSCRIPTION)


👍

I'd rather call it ANY_EVENT

mostlyobvious · 2019-09-17T16:53:32Z

ruby_event_store/lib/ruby_event_store/subscription.rb

@@ -1,6 +1,8 @@
 module RubyEventStore
  class Subscription
    def initialize(subscriber, event_types = [GLOBAL_SUBSCRIPTION], store: nil)
+      raise SubscriberNotExist, "subscriber must exists" unless subscriber


mostlyobvious · 2019-09-17T16:55:30Z

ruby_event_store/lib/ruby_event_store/subscription.rb

@@ -15,6 +17,10 @@ def unsubscribe
      event_types.each{ |type| @store.delete(self, type) } if @store
    end

+    def global?
+      event_types.include?(GLOBAL_SUBSCRIPTION)


Is it always event_types == [GLOBAL_SUBSCRIPTION] or do we allow a Subscription to be both global and for some particular type?

mostlyobvious · 2019-09-17T17:04:04Z

ruby_event_store/lib/ruby_event_store/spec/subscription_store_lint.rb

+
+  specify do
+    expect(subscription_store.all).to eq []
+    expect(subscription_store.all_for(FirstEvent)).to eq []


mostly ok but since we allow different event instances I think I'd prefer to pass #type or at least having these shared examples run on different Event implementation

def first_type FirstEvent.new.type end

mostlyobvious · 2019-09-17T17:06:31Z

ruby_event_store/lib/ruby_event_store/subscription.rb

    def initialize(subscriber, event_types = [GLOBAL_SUBSCRIPTION], store: nil)
-      raise SubscriberNotExist, "subscriber must exists" unless subscriber
+      raise SubscriberNotExist, 'subscriber must exists' unless subscriber


mostlyobvious · 2019-09-17T17:09:00Z

ruby_event_store/lib/ruby_event_store/subscription.rb

@@ -41,7 +65,7 @@ def inspect
    def ==(other)
      other.instance_of?(self.class) &&
        other.event_types.eql?(event_types) &&
-        other.subscriber.eql?(subscriber)
+        other.subscriber.equal?(subscriber)


Are you sure we want to check if it is the same instance in memory?

mostlyobvious · 2019-09-17T17:33:12Z

Please consider #135

mpraglowski added 10 commits August 9, 2019 14:52

Refactor subscriptions

39bc7ad

Remove thread specific subscriptions, replace with more generic LocalSubscriptions & GlobalSubscription with concurent subscription store. Allow to pass local & global store classes to Subscriptions.

Subscription storage is optional

8ff0e36

All dispatchers & schedulers receive now subscription object instead …

972be0a

…of subscriber

Single subscriptions store is enough

b693e49

Subscription global or assigned to specific type - it's just an imple…

33f34cb

…mentation detail

Minor improvement & fixes

2c9e3aa

Docs & lint/specs for subscriptions store

554cdbe

Docs & specs for Subscription object

4f76f27

No more local & global subscriptions, there are main subscription sto…

e18ceb3

…re & thread subscription (temporary subscriptions) stores only.

mpraglowski added 2 commits August 16, 2019 17:04

Do not need that anymore

c8dedab

Simplify subscriptions store API

29e62b4

mpraglowski added 7 commits August 16, 2019 17:44

Must be excluded from mutation tests as test where it could be trigge…

acc8184

…red is not executed during mutation tests

Kill mutant

2bd6c6d

Typo fix

9aefcdf

It does not make sense to require subscription store to be chainable

636cafb

Fix subscriptions store lint namespace

ff945e7

Add instrumented subscriptions store

cec639c

Use store factory for thread store instead of just a class

b82767b

It will allow better control on how thread store is created.

Again, keep Ruby 2.4 compatibility for now

570590f

FrozenError has been introduced in Ruby 2.5

mpraglowski added 3 commits September 6, 2019 10:48

Kill mutations

7140093

Add frozen_string_literal: true in all changed files

25dd989

Document subscriptions provider class

4ba25e3

mostlyobvious reviewed Sep 17, 2019

View reviewed changes

joelvh mentioned this pull request Mar 19, 2020

Q: Make self subscribing Objects in rails, Class load issues #716

Closed

mostlyobvious closed this Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions redesigned #665

Subscriptions redesigned #665

mpraglowski commented Aug 16, 2019

joelvh commented Aug 16, 2019

mpraglowski commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

mpraglowski commented Aug 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019 •

edited

mostlyobvious Sep 17, 2019

mostlyobvious Sep 17, 2019

mostlyobvious commented Sep 17, 2019

Subscriptions redesigned #665

Subscriptions redesigned #665

Conversation

mpraglowski commented Aug 16, 2019

joelvh commented Aug 16, 2019

mpraglowski commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

joelvh commented Aug 16, 2019

mpraglowski commented Aug 17, 2019

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

Choose a reason for hiding this comment

mostlyobvious Sep 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mostlyobvious commented Sep 17, 2019

mostlyobvious Sep 17, 2019 •

edited