Event Store v8 #354

prolic · 2018-12-22T17:02:26Z

Common classes and interfaces for Event Store v8

This will be adopted by

event-store-client for async TCP connections
event-store-http-client for HTTP connections
all upcoming (No-)SQL implementations

Merry X-Mas

Edit:
Should we remove the Exception suffix?

codeliner · 2018-12-22T21:50:07Z

Cool! I'll review after X-Mas, though.
Merry X-Mas @prolic 🎄 🎅

danizord

Great work! It seems that v8 brings a lot of new features and is way more robust :)

Just some points:

I'm wondering if this abstraction layer is not too big for now. I mean, some features like subscription statistics and user management will be a bit hard to implement in (No-)SQL implementations. I'd suggest to reduce this surface API to the minimum viable for the first 8.x release and start adding new features in next minor versions.
Having separate sync and async APIs seems to hurt readability and maintainability. Why not always return promises even when using sync connections?
For (No-)SQL, I think it will be easier to have different API for projections instead of parsing and interpreting JS code in PHP :P
How can the community help to write the new implementations? How about creating some issues as detailed tasks, so that we can parallelize some work? :)

Again, thanks for the great work. Merry Christmas!

README.md

src/AsyncEventStoreCatchUpSubscription.php

danizord · 2018-12-24T12:49:38Z

src/AsyncEventStorePersistentSubscription.php

+     */
+    public function fail(
+        ResolvedEvent $event,
+        PersistentSubscriptionNakEventAction $action,


Wow, I love it! 👏👏

danizord · 2018-12-24T12:58:13Z

src/AsyncPersistentSubscriptionDropped.php

+
+use Throwable;
+
+interface AsyncPersistentSubscriptionDropped


How about suffixing those with Handler?

I don't like that idea. Makes the names even longer.

Mhmm, I don't care about the length. This name is failing to express the purpose of this class :/

danizord · 2018-12-24T13:03:44Z

src/CatchUpSubscriptionSettings.php

+        return $this->verboseLogging;
+    }
+
+    public function enableVerboseLogging(): self


Suggested change

public function enableVerboseLogging(): self

public function withEnabledVerboseLogging(): self

To clarify immutability?

Don't like it.

danizord · 2018-12-24T13:21:23Z

src/EventData.php

+        return $this->eventType;
+    }
+
+    public function isJson(): bool


How about creating a JsonEventData class and determine it by type? Also, it could have some named constructor that receives data and metadata as array and encodes them internally.

No, that flag is very important for (No-)Sql implementations later. It's also the same in the original Event Store .NET Client. You can create a JsonEventData class in userland if you want to.

danizord · 2018-12-24T13:25:41Z

src/EventReadStatus.php

+        'Success' => 0,
+        'NotFound' => 1,
+        'NoStream' => 2,
+        'StreamDeleted' => 3,


You can reuse the constants here as value (same for other enum classes).

That was auto-generated by prolic/fpp. We can change that, of course, but for now I have more important things to do. You can send a PR though, would love it.

src/Exception/PersistentSubscriptionCommandFailedException.php

danizord · 2018-12-24T13:44:00Z

src/ExpectedVersion.php

+    // The stream being written to should not yet exist. If it does exist treat that as a concurrency problem.
+    public const NO_STREAM = -1;
+    // The stream should exist and should be empty. If it does not exist or is not empty treat that as a concurrency problem.
+    public const EMPTY_STREAM = -1;


Given the comment, it probably should not have the same value as NO_STREAM :)

Yes it should! I know it's weird, but that's the way it is. I can't change the implementation details of the original Event Store.

The comments tell that the event store behaves differently in each case, but how can it identify which one is being used? Maybe the comments are wrong then.

@danizord You only care about the constant name. What ever the value might be, it's not your problem to deal with.

No problem, just found this bug and wanted to help. It's impossible to branch behavior based on 2 flags that have the same value :)

We cannot change the values at all, really sorry.

I see, so you'll probably need to change the comments.

Why that? The comment declares exactly when to use which constant. Even if the value is the same, so what?

It also declares how the event store behaves in each case: NO_STREAM throws exception if the stream exists, while EMPTY_STREAM throws if the stream does not exist.

PR is welcome.

danizord · 2018-12-24T14:02:18Z

src/PersistentSubscriptionSettings.php

+
+    public static function create(): PersistentSubscriptionSettingsBuilder
+    {
+        return new PersistentSubscriptionSettingsBuilder(


Instead of having builders, I think it would be easier to have empty constructor, put these default values inline in property declaration and provide with* methods.

Don't like that idea.

Co-Authored-By: prolic <saschaprolic@googlemail.com>

prolic · 2018-12-25T07:49:02Z

Just some points:

I'm wondering if this abstraction layer is not too big for now. I mean, some features like subscription statistics and user management will be a bit hard to implement in (No-)SQL implementations. I'd suggest to reduce this surface API to the minimum viable for the first 8.x release and start adding new features in next minor versions.

For SQL implementations we can simply not implement the user management API is a first release, if it's too time consuming. However the interface exists, no matter if you implement it or not.

Having separate sync and async APIs seems to hurt readability and maintainability. Why not always return promises even when using sync connections?

This would give the false impression that all implementations are async, when they are clearly not. As of now there is for example no async driver for mongodb, except some very old experimental one.

For (No-)SQL, I think it will be easier to have different API for projections instead of parsing and interpreting JS code in PHP :P

Yes, that's correct, that's why we will be using PHP code instead.

How can the community help to write the new implementations? How about creating some issues as detailed tasks, so that we can parallelize some work? :)

I'm working on this since April this year and did everything alone, except for some reviews and bug reports from some people (thanks guys, you know who you are!). I also created most of the v7 implementation on my own. So realistically I don't expect much contribution from the community and therefore the extra work of writing lots of tickets that I do all by myself doesn't justify.

However if you or anyone else is willing to help out, hit me in the prooph-chat and we can discuss how you can help and I can give explanations on how some implementation details have to look like.

As of now, I like to finish the event-store-client and event-store-http-client first, before I start on the SQL implementations. I use them both already in production.

remove v7

10c0eeb

prolic added the BC break label Dec 22, 2018

prolic requested review from sandrokeil, codeliner and a team December 22, 2018 17:02

initial v8 implementation

3ac5fb9

prolic force-pushed the initial_v8 branch from fe95a60 to 3ac5fb9 Compare December 22, 2018 17:08

remove php-cs-fixer from composer json for 7.3 support

543a30e

prolic mentioned this pull request Dec 22, 2018

php-fpm "oops, unknown child exited with 1" prooph/event-store-client#68

Closed

prolic added 4 commits December 23, 2018 22:49

add AsyncEventStoreTransactionConnection interface

e0493cb

move listener handler away from internal namespace

02c2cea

make EventStoreSubscription an abstract class

320cc28

fix EventStoreSubscription

037f4be

prolic mentioned this pull request Dec 23, 2018

use prooph/event-store v8 as core prooph/event-store-client#74

Merged

prolic added 5 commits December 24, 2018 01:29

fix transaction handling

34f4968

fix EventStoreCatchUpSubscription interfaces

9688a38

php cs fix

37e050e

move event handlers for asyncconnections here

af69381

fix EventStorePersistentSubscription interface

980e712

danizord suggested changes Dec 24, 2018

View reviewed changes

danizord and others added 2 commits December 25, 2018 15:27

Update README.md

646e48d

Co-Authored-By: prolic <saschaprolic@googlemail.com>

clean up exceptions

714f919

prolic added 2 commits December 25, 2018 23:15

move cluster classes here

777302b

fix projections api

9679bab

prolic mentioned this pull request Dec 25, 2018

use prooph/event-store v8 as core prooph/event-store-http-client#9

Merged

prolic added 3 commits December 29, 2018 22:19

remove settings from interfaces

c680c58

update WrongExpectedVersionException

58b431d

update WrongExpectedVersionException

1a26f1d

update WrongExpectedVersionException

fa207de

codeliner approved these changes Dec 30, 2018

View reviewed changes

prolic merged commit 523a774 into master Dec 30, 2018

prolic deleted the initial_v8 branch December 30, 2018 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event Store v8 #354

Event Store v8 #354

prolic commented Dec 22, 2018 •

edited

Loading

codeliner commented Dec 22, 2018

danizord left a comment

danizord Dec 24, 2018

danizord Dec 24, 2018 •

edited

Loading

prolic Dec 25, 2018

danizord Dec 30, 2018

danizord Dec 24, 2018

prolic Dec 25, 2018

danizord Dec 24, 2018

prolic Dec 25, 2018

danizord Dec 24, 2018

prolic Dec 25, 2018

danizord Dec 24, 2018

prolic Dec 25, 2018

danizord Dec 30, 2018

prolic Dec 30, 2018

danizord Dec 30, 2018

prolic Dec 30, 2018

danizord Dec 30, 2018

prolic Dec 30, 2018

danizord Dec 30, 2018

prolic Dec 30, 2018

danizord Dec 24, 2018

prolic Dec 25, 2018

prolic commented Dec 25, 2018

	public function enableVerboseLogging(): self
	public function withEnabledVerboseLogging(): self

Event Store v8 #354

Event Store v8 #354

Conversation

prolic commented Dec 22, 2018 • edited Loading

codeliner commented Dec 22, 2018

danizord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danizord Dec 24, 2018 • 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

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

Choose a reason for hiding this comment

prolic commented Dec 25, 2018

prolic commented Dec 22, 2018 •

edited

Loading

danizord Dec 24, 2018 •

edited

Loading