Add queue-based event pub to make sure events are published in order #97

derekkraan · 2017-10-30T13:31:50Z

Run the included spec on master to see what happens without the new EventPublisher

lvonk · 2017-10-30T13:43:52Z

lib/sequent/core/event_publisher.rb

+          configuration.event_handlers.each do |handler|
+            begin
+              handler.handle_message event
+            rescue


This rescue can be moved down, this makes the code a bit more readable

def process_events rescue end

We can't do that since we don't have a reference to the event at the method body level.

Doh, you are right

lvonk · 2017-10-30T14:12:56Z

lib/sequent/core/event_publisher.rb

+        Thread.current[:events_queue] ||= Queue.new
+      end
+
+      def skip_if_locked(&block)


Maybe a better name is skip_if_publishing or skip_if_not_main_thread

lvonk

I like this a lot 👍 . Makes the intended way of publishing much more clear and explicit.

lvonk · 2017-10-30T20:21:58Z

lib/sequent/core/event_publisher.rb

+module Sequent
+  module Core
+    #
+    # EventPublisher ensures that, for every thread, events will be published in the order in which they are meant to be published.


I think "... will be published breadth (or width) first" is better.

To clarify, with breadth first I mean: first all first level events than the consequences of those events.

lvonk · 2017-10-30T20:31:04Z

lib/sequent/core/event_publisher.rb

+    #
+    # If you do not want this, you are free to implement your own version of EventPublisher and configure sequent to use it.
+    #
+    class EventPublisher


I am thinking to split this in 2 concerns. This might help in understanding the concepts better and might be easier to extend with other strategies. This also allows us to support the current behavior and be backward compatible.

The Events queue with is basically process_events

The FIFO strategy" which is basically events.each { |event| events_queue.push(event) }.

When someone want LIFO (or depth first) the event should be prepended to the Queue.

I think we should hold off on implementing something like this until someone asks for it / needs it.

lvonk · 2017-10-30T20:32:17Z

lib/sequent/core/event_publisher.rb

+      private
+
+      def events_queue
+        Thread.current[:events_queue] ||= Queue.new


Queue only supports FIFO I think. We can safely use Array here to support both FIFO and LIFO.

I think we should hold off on implementing something like this until someone asks for it / needs it.

We are changing the current behavior so I think it is good to support both cases and the current one as default, with maybe a warning message.

Neither of those scenarios is the current one.

Yes you are right... Okay, let's communicate this in the changelog when we release.

lvonk · 2017-10-30T20:35:09Z

lib/sequent/core/command_service.rb

-          end
-        ensure
-          repository.clear
+        commands.each do |command|


Can be a one-liner: each. {}

lvonk · 2017-10-30T20:37:43Z

lib/sequent/core/command_service.rb

@@ -57,6 +46,31 @@ def remove_event_handler(clazz)

      private

+      def process_commands


Can we extract this to a separate class like the EventPublisher. Both are quite similar in intent and behavior. Same suggestion made for the EventPublisher also apply here then :-)

CommandService is a separate class like EventPublisher already.

derekkraan · 2017-11-01T09:47:26Z

@erikrozendaal @s0meone @bjpbakker feedback? We will be merging this shortly.

bjpbakker · 2017-11-01T10:30:58Z

I do like this change, it solves a bunch of problems with published event ordering.

That said, current behavior is changed quite a lot, and may actually break current implementations. Most likely the tests won't cover this (because often they simply assert published events). That makes it tough to keep production code bases that are not in active development up to date with sequent.

Can we provide the current behavior via a configuration option?

derekkraan · 2017-11-01T10:45:48Z

This behaviour is configurable to a certain degree: One is free to simply implement her own version of EventPublisher and configure Sequent to use that with Sequent.configuration.event_publisher = YourEventPublisher. This is a couple of steps beyond trivial, but I feel that it's not an inordinate amount of work.

I'd rather we not officially support the legacy behaviour and the code that goes along with it.

If we get questions about this I'd be more than happy to help people gut EventPublisher and CommandService to suit their needs.

derekkraan · 2017-11-01T10:56:05Z

I think also that we should release this with a minor level version number increase to indicate that there is new functionality being introduces. This would be version 1.1.0.

bjpbakker · 2017-11-01T11:00:18Z

If we choose to no longer support the current order of event publishing (and stick with semantic versioning) we should bump to 2.0. This change currently is not backwards compatible.

derekkraan · 2017-11-01T11:08:20Z

That depends on your definition of "backwards compatible". Every change is potentially backwards incompatible, yet we don't increment major version number for every single bugfix.

The question is whether the new ordering is compatible with the old ordering. The answer is: that depends on how our users have implemented their systems and to what degree they are dependent on the old ordering. I suspect that most of them are not, making it for most (or all) users a backwards compatible change.

Or do you suspect that this will break functionality for a non-trivial percentage of Sequent users?

erikrozendaal · 2017-11-01T21:10:19Z

Running additional (event handler) generated commands in the same transaction may lead to huge, complicated transactions. This may not be what we want. But I guess handling new commands in separate transactions requires a lot of additional infrastructure (persistent queues, background jobs, etc).

So I guess this is fine, but I think we should discourage people from generating new commands in event handlers for immediate execution.

derekkraan · 2017-11-02T14:09:27Z

@bjpbakker let's talk about version numbers on Friday. Merging this since that isn't a blocking issue.

Add queue-based event pub to make sure events are published in order

0b0225b

derekkraan force-pushed the event_handler_queue branch from 58be6ab to 0b0225b Compare October 30, 2017 13:32

lvonk reviewed Oct 30, 2017

View reviewed changes

derekkraan added 2 commits October 30, 2017 14:45

Only process events at the highest level (don't enter this loop twice)

b2e0441

Synchronize events within a single thread

1de7799

derekkraan force-pushed the event_handler_queue branch from 9344491 to 1de7799 Compare October 30, 2017 14:01

derekkraan assigned derekkraan, s0meone and erikrozendaal and unassigned derekkraan, s0meone and erikrozendaal Oct 30, 2017

lvonk reviewed Oct 30, 2017

View reviewed changes

derekkraan requested review from s0meone, lvonk, erikrozendaal and bjpbakker October 30, 2017 14:13

derekkraan added 4 commits October 30, 2017 15:14

Make name more descriptive

331dc5e

Comment on the right level

959fe1f

Clear queue if there is a problem

c3d4d6d

Process additional commands after the current command has finished

4b3c5cb

lvonk requested changes Oct 30, 2017

View reviewed changes

Cosmetic changes

4e2453e

derekkraan force-pushed the event_handler_queue branch from ea52ad9 to e7eb2fc Compare November 1, 2017 08:58

derekkraan added 2 commits November 1, 2017 09:59

efactor code to make it easier to extract functionality at a later date

ed7d206

Require 'tmpdir'

1d55cc4

derekkraan force-pushed the event_handler_queue branch from e7eb2fc to 1d55cc4 Compare November 1, 2017 08:59

lvonk approved these changes Nov 1, 2017

View reviewed changes

derekkraan removed the request for review from s0meone November 1, 2017 12:02

derekkraan merged commit 29078cb into master Nov 2, 2017

stephanvd deleted the event_handler_queue branch June 8, 2018 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add queue-based event pub to make sure events are published in order #97

Add queue-based event pub to make sure events are published in order #97

derekkraan commented Oct 30, 2017 •

edited

Loading

lvonk Oct 30, 2017

derekkraan Oct 30, 2017 •

edited

Loading

lvonk Oct 30, 2017

lvonk Oct 30, 2017

lvonk left a comment

lvonk Oct 30, 2017

lvonk Oct 30, 2017

derekkraan Nov 1, 2017

lvonk Oct 30, 2017

derekkraan Nov 1, 2017 •

edited

Loading

lvonk Nov 1, 2017

derekkraan Nov 1, 2017

lvonk Nov 1, 2017

lvonk Oct 30, 2017

lvonk Oct 30, 2017

derekkraan Nov 1, 2017

derekkraan commented Nov 1, 2017

bjpbakker commented Nov 1, 2017

derekkraan commented Nov 1, 2017

derekkraan commented Nov 1, 2017

bjpbakker commented Nov 1, 2017

derekkraan commented Nov 1, 2017

erikrozendaal commented Nov 1, 2017

derekkraan commented Nov 2, 2017

		@@ -57,6 +46,31 @@ def remove_event_handler(clazz)

		private

		def process_commands

Add queue-based event pub to make sure events are published in order #97

Add queue-based event pub to make sure events are published in order #97

Conversation

derekkraan commented Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

derekkraan Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvonk left a 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

derekkraan Nov 1, 2017 • edited Loading

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

derekkraan commented Nov 1, 2017

bjpbakker commented Nov 1, 2017

derekkraan commented Nov 1, 2017

derekkraan commented Nov 1, 2017

bjpbakker commented Nov 1, 2017

derekkraan commented Nov 1, 2017

erikrozendaal commented Nov 1, 2017

derekkraan commented Nov 2, 2017

derekkraan commented Oct 30, 2017 •

edited

Loading

derekkraan Oct 30, 2017 •

edited

Loading

derekkraan Nov 1, 2017 •

edited

Loading