Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Message header interface for more decoupling and type safety #496

Merged
merged 20 commits into from
Mar 7, 2024

Conversation

DanielBadura
Copy link
Member

No description provided.

@DanielBadura DanielBadura changed the base branch from 2.3.x to 3.0.x February 10, 2024 10:59
Copy link
Member

@DavidBadura DavidBadura left a comment

Choose a reason for hiding this comment

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

That's kind of how I thought it too 👍

src/EventBus/Message.php Outdated Show resolved Hide resolved
src/EventBus/Message.php Outdated Show resolved Hide resolved
src/EventBus/Message.php Outdated Show resolved Hide resolved
src/Outbox/OutboxHeader.php Outdated Show resolved Hide resolved
src/EventBus/Message.php Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 13, 2024

Hello 👋

here is the most recent benchmark result:

SnapshotsBench
==============

+----------------------------------------+--------------------+--------------------+-----------------+------------+
|                                        | time (kde mode)                         | memory                       |
+----------------------------------------+--------------------+--------------------+-----------------+------------+
| subject                                | Tag: <current>     | Tag: base          | Tag: <current>  | Tag: base  |
+----------------------------------------+--------------------+--------------------+-----------------+------------+
| benchLoad10000EventsMissingSnapshot () | 159.050ms (±0.00%) | 167.243ms (±0.00%) | 38.681mb        | 38.681mb   |
| benchLoad10000Events ()                | 530.000μs (±0.00%) | 521.700μs (±0.00%) | 38.681mb        | 38.681mb   |
+----------------------------------------+--------------------+--------------------+-----------------+------------+

SimpleSetupBench
================

+----------------------------------------+--------------------+--------------------+-----------------+------------+
|                                        | time (kde mode)                         | memory                       |
+----------------------------------------+--------------------+--------------------+-----------------+------------+
| subject                                | Tag: <current>     | Tag: base          | Tag: <current>  | Tag: base  |
+----------------------------------------+--------------------+--------------------+-----------------+------------+
| benchLoad10000Events ()                | 158.057ms (±0.00%) | 171.136ms (±0.00%) | 38.684mb        | 38.663mb   |
| benchSave1Event ()                     | 871.300μs (±0.00%) | 901.100μs (±0.00%) | 38.684mb        | 38.663mb   |
| benchSave10000Events ()                | 473.695ms (±0.00%) | 485.517ms (±0.00%) | 38.951mb        | 38.940mb   |
| benchSave10000Aggregates ()            | 8.605s (±0.00%)    | 8.987s (±0.00%)    | 38.684mb        | 38.663mb   |
| benchSave10000AggregatesTransaction () | 7.381s (±0.00%)    | 7.398s (±0.00%)    | 38.684mb        | 38.663mb   |
+----------------------------------------+--------------------+--------------------+-----------------+------------+

SplitStreamBench
================

+-------------------------+--------------------+--------------------+-----------------+------------+
|                         | time (kde mode)                         | memory                       |
+-------------------------+--------------------+--------------------+-----------------+------------+
| subject                 | Tag: <current>     | Tag: base          | Tag: <current>  | Tag: base  |
+-------------------------+--------------------+--------------------+-----------------+------------+
| benchLoad10000Events () | 5.309ms (±0.00%)   | 5.597ms (±0.00%)   | 42.549mb        | 41.963mb   |
| benchSave10000Events () | 624.281ms (±0.00%) | 631.580ms (±0.00%) | 42.826mb        | 42.250mb   |
+-------------------------+--------------------+--------------------+-----------------+------------+

ProjectionistBench
==================

+---------------------------+-----------------+-----------------+-----------------+------------+
|                           | time (kde mode)                   | memory                       |
+---------------------------+-----------------+-----------------+-----------------+------------+
| subject                   | Tag: <current>  | Tag: base       | Tag: <current>  | Tag: base  |
+---------------------------+-----------------+-----------------+-----------------+------------+
| benchHandle10000Events () | 3.294s (±0.00%) | 3.341s (±0.00%) | 39.079mb        | 39.102mb   |
+---------------------------+-----------------+-----------------+-----------------+------------+

This comment gets update everytime a new commit comes in!

@DanielBadura DanielBadura added enhancement New feature or request BC-Break labels Feb 16, 2024
@DanielBadura DanielBadura self-assigned this Feb 16, 2024
@DanielBadura DanielBadura added this to the 3.0.0 milestone Feb 16, 2024
@DanielBadura DanielBadura force-pushed the message-header-interface branch 2 times, most recently from 1684cb5 to d2f090a Compare February 27, 2024 12:33
@DanielBadura
Copy link
Member Author

Do we even need the SerializedHeader dto?

Copy link
Member

@DavidBadura DavidBadura left a comment

Choose a reason for hiding this comment

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

Nice work 👍 I still have a few wishes :D

src/Attribute/HeaderIdentifier.php Outdated Show resolved Hide resolved
src/EventBus/Header.php Outdated Show resolved Hide resolved
src/EventBus/Message.php Outdated Show resolved Hide resolved
src/EventBus/Serializer/HeadersSerializer.php Outdated Show resolved Hide resolved
src/EventBus/Serializer/SerializedHeader.php Outdated Show resolved Hide resolved
@@ -49,6 +50,10 @@ public function setUp(): void
$this->store = new DoctrineDbalStore(
$connection,
DefaultEventSerializer::createFromPaths([__DIR__ . '/BasicImplementation/Events']),
DefaultHeadersSerializer::createFromPaths([
Copy link
Member

Choose a reason for hiding this comment

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

We should add the default headers internally, so the developer no need to know, that he need a specific path. And we should do this without a path: we should add our headers directly. auto-discovery should only used in user space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'm not sure about that. Due to that it's more masked for the user that this is happening and if he implements his own version of the HeadersSerializer he needs to know this internal logic in our DefaultHeadersSerializer.

But if we we do this then i think we should keep it as auto discover due two reasons:

  1. We cannot forget to add headers then there
  2. No need for extra logic

Copy link
Member

Choose a reason for hiding this comment

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

  1. I don't think it's a relevant point that you forget it, since we'll hopefully cover it with tests so that everything fits.

  2. We can push the logic further down to MessageHeaderRegistry.

class MessageHeaderRegistry
{
    // ...
    
    /** @param array<string, class-string<Header>> $headerNameToClassMap */
    public static function withInternalHeaders(array $headerNameToClassMap): self
    {
        $internalHeaders = [
            'aggregate' => AggregateHeader::class,
            // ...
        ];
    
        return new self($headerNameToClassMap + $internalHeaders);
    }
}

This means it cannot be forgotten in the serializer.

To the point that no extra logic is necessary: It's an array merge, there isn't that much extra logic. On the other hand, we have an IO search in our code, which is not necessary because we know exactly where the classes are.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we are duplicating the naming code 🤔
So i think, if we move it, it should land in AttributeMessageHeaderRegistryFactory but still feels weird.

        $classes = (new ClassFinder())->findClassNames($paths);
        $classes = array_merge([AggregateHeader::class, ...], $classes);

Copy link
Member

Choose a reason for hiding this comment

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

Then we are duplicating the naming code 🤔

I do not understand that.

For me, MessageHeaderRegistry would be the more correct place. I wouldn't put it any further down.

Copy link
Member

Choose a reason for hiding this comment

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

If you mean the attributes. I would remove these from the internal headers because the attributes are only used for configuration. And since we define the internal headers differently, they have no meaning there. The attribute is for user space.

@DanielBadura DanielBadura marked this pull request as ready for review March 7, 2024 10:54
@DavidBadura DavidBadura merged commit 486ab70 into 3.0.x Mar 7, 2024
32 of 33 checks passed
@DavidBadura DavidBadura deleted the message-header-interface branch March 7, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC-Break enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants