Implement merge strategy based on TYPE+TARGET

[Marenz]

No description provided.

[Marenz]

Based on #99

[llucax]

In summary:

• I would make the MergeStrategy interface public and simplify it as suggested.
• Make everything needed by merge strategies available as public symbols.
• I would make merge strategies take any state they need via the constructor, not the filter function.
• Expose the cached dispatches via the Dispatcher directly.

[Marenz]

Updated @llucax

[Marenz]

I solved avoiding passing the service now by just using the type instead

[llucax]

Why is this still returning a callable and is not the callable itself? 🤔

[llucax]

I still prefer the approach of instantiating the merge strategy from outside and passing the instance for a couple of reasons:

1. Flexibility. A merge instance could potentially need to keep more state than just the scheduler, if we instantiate it inside the function we remove that flexibility of allowing merge strategies have the state they want.
2. Reuse: We could potentially reuse the same merge strategy instance with many different actors (this is minor, the above is a stronger point, but still).

If the idea is to make the typical use easier, I liked the idea of adding a wrapper for common merge strategies in Dispatcher for example, like Dispatcher.mergeByWhatever(), but I think the design/architecture of merge strategies in general should be flexible.

Then I also don't think it is much worse from the user perspective to use MergeByWhatever(dispatcher) than dispatcher.mergeByWhatever(), and it might be generally useful to have dispatcher.dispatches exposed publicly as read-only.

[Marenz]

If the idea is to make the typical use easier, I liked the idea of adding a wrapper for common merge strategies in Dispatcher for example, like Dispatcher.mergeByWhatever(), but I think the design/architecture of merge strategies in general should be flexible.

I thought about this approach, but at the end it wouldn't save anything because then you have to type

• dispatcher.new_running_state_event_receiver("TEST", dispatcher.mergeByExample) instead of
• dispatcher.new_running_state_event_receiver("TEST", MergeByExample(dispatcher))

So that seemed a bit useless.

Only a full wrapper like

• dispatcher.new_rst_event_receiver_merged_by_example("TEST")

would make some usage difference..

[llucax]

Agreed. So combined to what we discussed in #104 I guess we agree that the end goal would be to end up with: dispatcher.manage(type="FCR", actor_factory=.., strategy=MergeByType(dispatcher)).

On the other hand... If we are merging, I guess you always need to have the full list of dispatches, otherwise how would you merge? People can still create merge strategies that require more state, but I guess the list of available dispatches is the minimum you need to do any merging on the typical case.

What about?

class MergeStrategy(ABC):
    @abstractmethod
    def filter(self, all_dispatches: Map[int, Dispatch], current_dispatch: Dispatch) -> bool: ...

...
        if merge_strategy:
            receiver = receiver.filter(functools.partial(merge_strategy.filter, self._dispatches))
            # or lambda d: merge_strategy.filter(self._dispatches, d)

[Marenz]

Updated. It doesn't have the unified method manage yet, I intend to add that in a later PR

[llucax]

A few more comments, but looking preeeeety goooood!

[Marenz]

Updated, addressed all comments

[llucax]

Amazing! 🎉