Skip to content

Conversation

ziccardi
Copy link
Contributor

@ziccardi ziccardi commented May 29, 2025

Background

Previously, observers were invoked synchronously by the routine manager. This meant that if an observer was slow or blocking, it could negatively impact the performance and responsiveness of the entire routine management system.

Enhancement

This pull request introduces NewAsyncRoutineObserver, a wrapper that allows any existing observer to operate asynchronously. With this change:

  • Observers are decoupled from the main routine manager execution flow.
  • Slow or blocking observers no longer delay routine processing.
  • The asynchronous observer uses a buffered channel to queue events and processes them in a separate goroutine.

Key Benefits

  • Improved Performance: The routine manager is no longer affected by the speed of individual observers.
  • Drop-in Compatibility: Existing observer implementations can be wrapped without modification.
  • Configurable Behavior: Buffer size and delivery guarantees can be tuned via options.

Example

// Wrap an existing observer to make it asynchronous
asyncObserver := NewAsyncRoutineObserver(existingObserver, WithChannelSize(32))

This ensures that observer notifications are handled off the main execution path, improving overall system robustness and scalability.

@tzvatot
Copy link
Contributor

tzvatot commented Jun 3, 2025

Couple of thoughts:

  1. We don't nervelessly have a requirement to run the observers async. While in some cases it make sense, and has value, the consumer of the framework can achieve that by creating a top level observer, that will notify a set of child-observers asynchronously. So I'm contemplating between keeping the framework simple and yet expandable, than introducing a level of complexity that we don't necessarily need, which also adds some risk.
  2. This PR does not have any tests attached. Given #1, maybe that is not required.

@ziccardi
Copy link
Contributor Author

Couple of thoughts:

1. We don't nervelessly have a requirement to run the observers async. While in some cases it make sense, and has value, the consumer of the framework can achieve that by creating a top level observer, that will notify a set of child-observers asynchronously. So I'm contemplating between keeping the framework simple and yet expandable, than introducing a level of complexity that we don't necessarily need, which also adds some risk.

2. This PR does not have any tests attached. Given `#1`, maybe that is not required.

I agree with your points. To address the concern about unnecessary complexity, I’ve updated the PR so that it no longer changes the core engine. Instead, I’ve introduced a NewAsyncRoutineObserver function, which allows any existing observer to be wrapped as an asyncRoutineObserver. This keeps the main framework simple and focused, while still providing an easy way for consumers to opt into asynchronous observer behavior if they need it.
Let me know if you have any further suggestions/concerns.

}
}

func (a *asyncRoutineObserver) startObserving() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this be getting ctx as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What usage of the context are you thinking about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants