Closed oqq closed 7 years ago
prolic
commented
7 years ago We did not release a final version, so BC breaks are still acceptable.
With the projection options that abstract, we can also use arrays for this purpose, there is no need to have a class anymore. But what I dislike, is the huge amount of constants added to the event-store implementation. Currently I am note sure, what's the best way to go.
oqq
commented
7 years ago Sure an array would also be fine.
But in that way we have a tool to provide some recurring instructions how: give me that or the default, test that array against my mandatories. Maybe this DTO could handle some of the constants which would be recurring over most projectors.
Where the constants are is not important. But the default values should be in constants instead of method argument defaults. Also for investigations where this values would be used constants are much better than simple strings for array keys. I like it to jump in PhpStorm around by clicking and searching for implemenations instead of searching for a string in whole code.
sandrokeil
commented
7 years ago Why does we not use an interop-config factory for the options. Or why not use an interop-config factory for the projections? An array as options is maybe not the best choice for mandatory constructor args.
I guess the best choice is to use a factory which implements interop-config for the projections so the config can also be generated. And simple check with assertions in contructor for mandatory fields in the options array. The factory can also define the options as constants or we use a specific option class for this.
PdoEventStoreProjectionOptions::OPTION_PDO_CONNECTION
or
PdoEventStoreProjectionFactory::OPTION_PDO_CONNECTION
public function __construct(EventStore $eventStore, string $name, array $options)In that way the factory could be bypassed and we have to re-test all values in the projection, right?
codeliner
commented
7 years ago @oqq good point, but...
I like it that current ProjectionOptions are type safe: https://github.com/prooph/event-store/blob/develop/src/Projection/ProjectionOptions.php#L34
Here you have a problem: https://gist.github.com/oqq/c709eae6d993b49d888d15703a466f30#file-projectionoptions-php-L30
It is not guaranteed that for example the connection is a PDOConnection: https://gist.github.com/oqq/c709eae6d993b49d888d15703a466f30#file-projectionoptions-php-L30
Maybe a simple options array combined with a interop-factory to create the projection is a good trade off, like suggested by @sandrokeil. A factory is already in use: https://github.com/prooph/pdo-event-store/blob/master/src/PostgresEventStore.php#L426
The factory could take an options array as argument and make use of the logic provided by interop-config. Bonus: we use interop-config across prooph components, so we use the same technique for projections. Less confusion, easier to explain.
Doing this we lose type safety of options but get more flexibility with less code. A trade off I can live with.
oqq
commented
7 years ago In a real application I would prefer to use container-interop provided factories in favor of the EventStore. But I would not like to return such a factory from an EventStore instance.
@prolic is there any chance to remove the projections completely from the EventStore interface? What was the point to let the EventStore query and projections aware? Is that not a to deep coupling we could avoid and slim down the interface?
@codeliner maybe we should differentiate between mandatory values, needed by the projection to could work. This values should be injected type safe (you´re right, that is an important fact I have completely ignored) in the projection instance. Best way is a separate factory with container-interop how suggested from @sandrokeil. I am full with you guys.
But there are also ProjectionRuntimeOptions, which could also be an array and used as method call argument in the run method to all projections. They should be mostly scalar, indeed integers.
sleep and persistBlockSize are such values. They could differ for every run process. And they could have default values known by the projector.
A ProjectionRunner how provided in the standard-projections package is such a service wich should also be aware of the ProjectionRuntimOptions
prolic
commented
7 years ago My thoughts:
1) Removing the projection options classes:
I am not sure about that, type safety is really important, if you don't want to have some weird side effects during usage. If someone wants to provide a PR that's worth to discuss, I would perhaps change my mind on that. I didn't found a better way to do this until now.
2) About removing the query / projections part from event store interface
I played around with this a lot, and I think what we currently have is the best way to go. Some reasons behind the decisions made (taken by example from pdo-event-store):
final class PdoEventStoreProjection implements Projection
{
// some properties
public function __construct(
EventStore $eventStore,
PDO $connection,
string $name,
string $eventStreamsTable,
string $projectionsTable,
int $lockTimeoutMs,
int $cacheSize,
int $persistBlockSize,
int $sleep
) {
// code
}
// more code
}Here you see what the current constructor is:
1) EventStore - this must be the correct event-store bound to the implementation, you are not allowed to mix InMemoryEventStore with PDOProjection - that would not work well. 2) PDO Connection - it's worth to note it must be the SAME connection the event-store uses, not a new one created seperately. 3) eventStreamsTable - this is internal information from the event-store, wrong configuration would break the projection, hard to get consistency here without reflection 4) projectionsTable - while thiis could be unique to projections, this information also comes from the event-store to answer questions like: get me all projection names, and so on
So in order to solve these problems, the event store is responsible for creating the projection instance - this solves a lot of those problems outlined above: passing correct event store instance, passing the internally used pdo connection, passing internal eventStreamsTable name and so on.
Next problem that comes up is, when using ActionEventEmitterEventStore you need to pass internal information of the wrapped event store implementation - a task that cannot be solved without reflection - or, like I decided to go, we add a getDefaultProjectionFactory() method to the event store interface. This way the ActionEventEmitterEventStore can get a factory from the wrapped event store implementation and pass itself (the correct event store implementation) to the projection.
This ends with a kind of bloated interface - but I don't think it's a problem at all. It's still all in responsibility of the event store:
I think is all fits well the event stores responsibilities.
oqq
commented
7 years ago @prolic this reasons makes sense, for the current implementation. Maybe we should go a huge step forward and remove the coupling from projections to concrete EventStore implementation.
What if I would store the "projectionsTable" in MongoDb or in memory? How I get this, the EventStore should provide a way to query events by:
And the Projection should only aware of Events.. Not where or how this events are stored at all. So a query could be abstract and a final class, and an EventStore should have a method to resolve such a query to events. Projections than build queries and request the EventStore.
Maybe I have not enough insights in the current pdo projections implementation and that all would not work how I guess. E.g I see not y the PdoProjection need the same connection how the PdoEventStore. I can not found where this required coupling comes from. All add and load methods in EventStore are not touching the projections at all. Same in other direction. The Projection don´t touch the event stream table. There is a lock on projections table, sure. But such a look could also be done in other implementations. And all requests the projection does are abstract and could resolved from every event store implementation. Such as:
But how I said already.. there is a big chance i have not read the code good enough to estimate if my proposal makes sense. If so.. sorry for annoying ;)
Maybe there is a way to implement one Projection class which could handles all EventStores.
prolic
commented
7 years ago oqq
commented
7 years ago Thats a query against EventStore which could be defined abstract easily. The Projection only need to know that only Events from a category should get fetched. Which could be reflected in a query.
So in projection I build an abstract query, with run i request all events against EventStore with this query.
In that way I could run a PdoEventStoreProjection against a RedisEventStore. Better than that, i doesn't need to implement a special Projection in all event store implementations at all. In most cases an InMemoryProjection is enough. If I am aware to lock a projection for sure.
Maybe an EventStore should only know about event storage.
prolic
commented
7 years ago If you like to do something in memory without persistance, use a query. It makes no sense to have a non-persistent projection.
Also you cannot get a list of running projections, stop a projection, and so on, when this information is no longer part of the event store.
For the projection storage only some key value like stuff is needed, which can be implemented easily in every database. That said, it also makes no sense to have a mongoprojection on top of pdo event store. The mongo part would only store current state of the projection, there is no benefit in doing so, while you lose the abilities to fetch projection names, stop, reset and delete projections from the event store.
That said, i am totally not convinced, we should take that road.
We can talk about projection options, when there is a PR which feels good and is ready to get discussed.
sandrokeil
commented
7 years ago @prolic I find @oqq considerations very interesting. To decouple creation of projections, we can use the existing PDO connection of the event store (we should remove creation of the connection in the factory). The config could be (taken from proophessor-do).
return [
'proophessor-do' => [
'pdo_connection' => [
'driver' => 'pdo_pgsql',
'user' => 'postgres',
'password' => '',
'host' => '127.0.0.1',
'dbname' => 'todo',
'port' => 5432,
],
],
// dependencies settings
'dependencies' => [
'factories' => [
\Prooph\EventStore\EventStore::class => [
'Prooph\EventStore\Pdo\Container\PostgresEventStoreFactory',
'default',
],
\My\Pdo\CategoryProjection::class => [
\My\Pdo\CategoryProjectionFactory::class, // retrieves $projectionsTable or $eventStreamsTable from event store instance
'default',
],
\My\Pdo\CategoryProjectionOptions::class => [
\My\Pdo\CategoryProjectionOptionsFactory::class, // can be merged to CategoryProjectionFactory or we create a new config entry projection_options
'my_category_projection',
],
],
],
// prooph settings
'prooph' => [
'projections' => [
'my_category_projection' => [
'connection' => 'pdo.connection',
'event_store' => \Prooph\EventStore\EventStore::class,
'cache_size' => 1234,
'any_option' => 'for the specific projection',
// or
'projection_options' => 'service name to the CategoryProjectionOptions',
],
],
'event_store' => [
'default' => [
'connection_service' => 'service name to the proophessor-do connection above (pdo.connection)',
'event_streams_table' => 'event_streams', // <- EventStore interface needs a getter for this
'projections_table' => 'projections', // <- EventStore interface needs a getter for this
'persistence_strategy' => 'Prooph\EventStore\Pdo\PersistenceStrategy\PostgresSingleStreamStrategy',
],
],
'service_bus' => [
'command_bus' => [
'plugins' => [
\Prooph\EventStoreBusBridge\TransactionManager::class,
],
],
],
],
];Maybe it looks complicated at the first glance, but we can remove all the createProjection*(), createQuery with corresponding getDefaul factory methods from the event store and it follows our container interop pattern at all. There are some miss configuration possibilities (wrong event store, wrong PDO connection) for sure, but we have only getDefault*Factory methods which can also be bypassed and without this it looks much cleaner. How many event stores is an application using, I guess only one, same for the connection, so the risk for misconfiguration is very low. /cc @codeliner
I can bring a PR if we want to go this way. I think this should work.
prolic
commented
7 years ago I am against additional getters for event stream and projection table on the event store interface.
Am 19.02.2017 23:32 schrieb "Sandro Keil" notifications@github.com:
@prolic https://github.com/prolic I find @oqq https://github.com/oqq considrations very interesting. To decouple creation of projections, we can use the existing PDO connection of the event store (we should remove creation of the connection in the factory https://github.com/prooph/pdo-event-store/blob/master/src/Container/AbstractEventStoreFactory.php#L83-L89). The config could be (taken from proophessor-do).
return [ 'proophessor-do' => [ 'pdo_connection' => [ 'driver' => 'pdo_pgsql', 'user' => 'postgres', 'password' => '', 'host' => '127.0.0.1', 'dbname' => 'todo', 'port' => 5432, ], ], // dependencies settings 'dependencies' => [ 'factories' => [ \Prooph\EventStore\EventStore::class => [ 'Prooph\EventStore\Pdo\Container\PostgresEventStoreFactory', 'default', ], \My\Pdo\CategoryProjection::class => [ \My\Pdo\CategoryProjectionFactory::class, // retrieves $projectionsTable or $eventStreamsTable from event store instance 'default', ], \My\Pdo\CategoryProjectionOptions::class => [ \My\Pdo\CategoryProjectionOptionsFactory::class, // can be merged to CategoryProjectionFactory or we create a new config entry projection_options 'my_category_projection', ], ], ], // prooph settings 'prooph' => [ 'projections' => [ 'my_category_projection' => [ 'connection' => 'pdo.connection', 'event_store' => \Prooph\EventStore\EventStore::class, 'cache_size' => 1234, 'any_option' => 'for the specific projection', // or 'projection_options' => 'service name to the CategoryProjectionOptions', ], ], 'event_store' => [ 'default' => [ 'connection_service' => 'service name to the proophessor-do connection above (pdo.connection)', 'event_streams_table' => 'event_streams', // <- EventStore interface needs a getter for this 'projections_table' => 'projections', // <- EventStore interface needs a getter for this 'persistence_strategy' => 'Prooph\EventStore\Pdo\PersistenceStrategy\PostgresSingleStreamStrategy', ], ], 'service_bus' => [ 'command_bus' => [ 'plugins' => [ \Prooph\EventStoreBusBridge\TransactionManager::class, ], ], ], ],];
Maybe it looks complicated at the first glance, but we can remove all the createProjection(), createQuery with corresponding getDefaul factory methods from the event store and it follows our container interop pattern at all. There are some miss configuration possibilities (wrong event store, wrong PDO connection) for sure, but we have only getDefaultFactory methods which can also be bypassed and without this it looks much cleaner. How many event stores is an application using, I guess only one, same for the connection, so the risk for misconfiguration is very low. /cc @codeliner https://github.com/codeliner
I can bring a PR if we want to go this way. I think this should work.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prooph/event-store/issues/265#issuecomment-280926435, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvAqk14TOpMgsnqXR9VIUQpa7Rw5qks5reGCIgaJpZM4MFGhH .
prolic
commented
7 years ago Also I don't understand what you have against $eventStore->createProjection(). I ask the event store to create me a projection for its event streams. Why should I ask the container for this stuff at all?
Am 19.02.2017 23:43 schrieb "Sascha-Oliver Prolic" < saschaprolic@googlemail.com>:
I am against additional getters for event stream and projection table on the event store interface.
Am 19.02.2017 23:32 schrieb "Sandro Keil" notifications@github.com:
@prolic https://github.com/prolic I find @oqq https://github.com/oqq considrations very interesting. To decouple creation of projections, we can use the existing PDO connection of the event store (we should remove creation of the connection in the factory https://github.com/prooph/pdo-event-store/blob/master/src/Container/AbstractEventStoreFactory.php#L83-L89). The config could be (taken from proophessor-do).
return [ 'proophessor-do' => [ 'pdo_connection' => [ 'driver' => 'pdo_pgsql', 'user' => 'postgres', 'password' => '', 'host' => '127.0.0.1', 'dbname' => 'todo', 'port' => 5432, ], ], // dependencies settings 'dependencies' => [ 'factories' => [ \Prooph\EventStore\EventStore::class => [ 'Prooph\EventStore\Pdo\Container\PostgresEventStoreFactory', 'default', ], \My\Pdo\CategoryProjection::class => [ \My\Pdo\CategoryProjectionFactory::class, // retrieves $projectionsTable or $eventStreamsTable from event store instance 'default', ], \My\Pdo\CategoryProjectionOptions::class => [ \My\Pdo\CategoryProjectionOptionsFactory::class, // can be merged to CategoryProjectionFactory or we create a new config entry projection_options 'my_category_projection', ], ], ], // prooph settings 'prooph' => [ 'projections' => [ 'my_category_projection' => [ 'connection' => 'pdo.connection', 'event_store' => \Prooph\EventStore\EventStore::class, 'cache_size' => 1234, 'any_option' => 'for the specific projection', // or 'projection_options' => 'service name to the CategoryProjectionOptions', ], ], 'event_store' => [ 'default' => [ 'connection_service' => 'service name to the proophessor-do connection above (pdo.connection)', 'event_streams_table' => 'event_streams', // <- EventStore interface needs a getter for this 'projections_table' => 'projections', // <- EventStore interface needs a getter for this 'persistence_strategy' => 'Prooph\EventStore\Pdo\PersistenceStrategy\PostgresSingleStreamStrategy', ], ], 'service_bus' => [ 'command_bus' => [ 'plugins' => [ \Prooph\EventStoreBusBridge\TransactionManager::class, ], ], ], ],];
Maybe it looks complicated at the first glance, but we can remove all the createProjection(), createQuery with corresponding getDefaul factory methods from the event store and it follows our container interop pattern at all. There are some miss configuration possibilities (wrong event store, wrong PDO connection) for sure, but we have only getDefaultFactory methods which can also be bypassed and without this it looks much cleaner. How many event stores is an application using, I guess only one, same for the connection, so the risk for misconfiguration is very low. /cc @codeliner https://github.com/codeliner
I can bring a PR if we want to go this way. I think this should work.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prooph/event-store/issues/265#issuecomment-280926435, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvAqk14TOpMgsnqXR9VIUQpa7Rw5qks5reGCIgaJpZM4MFGhH .
codeliner
commented
7 years ago Also I don't understand what you have against $eventStore->createProjection(). I ask the event store to create me a projection for its event streams. Why should I ask the container for this stuff at all?
@prolic There is nothing wrong with your POV. But, we had this discussion before if you remember? I also thought about a way to decouple projections from the store but had not enough time to really dig into it (fought with mongo-event-store v7 and gave up)
The problem is: The event-store has too many responsibilities now and that feels wrong somehow. It would be nicer to have an event-store that focuses on events and streams and provide a generic way to query those streams.
At the moment event-store v7 is a beast and it tends to become a god object, which is an alarm signal.
We should keep in mind that the event store must be scalable, so a projection SHOULD use another connection for example. When doing CQRS right you get the advantage that you can safely write to a primary and query from slaves. Same should be possible with projections.
Another consideration:
I would not say that there is only one event-store per application. Again, scalability concern.
If one of my aggregate types is under heavy load I might want to use a dedicated event-store just for those aggregates.
Now imagine that it would be possible to let a projection compose different event-stores to create an $all stream. Sounds like a valid use case for a distributed system.
@oqq and @sandrokeil make very good points here. It is a warning sign that we all feel the current event-store does too many things and projections are coupled with the event-store. We are in a beta phase and start using the new v7 version. So I think it is normal that this kind of discussions arise. I'd love to see a PR (can be WIP) that demonstrates an alternative approach. @prolic put a lot of work into the new version and the new features are great. Let's work together on the final bits so that we get v7 out!
prolic
commented
7 years ago We should keep in mind that the event store must be scalable, so a projection SHOULD use another connection for example. When doing CQRS right you get the advantage that you can safely write to a primary and query from slaves. Same should be possible with projections.
When you are using MySQL or PostgreSQL a master-slave setup can be done. Master is used for writing to the event streams, a slave can be used for reading event streams in projections.
I would not say that there is only one event-store per application.
In a lot of cases you end up with at least two event stores (probably not physical distinctions, but different used configurations). The main reason is, that if you have a stream-projection (project events from one stream to another) you might not want the same plugins in place, f.e. you just want to copy those events to another stream, not add some metadata again.
At the moment event-store v7 is a beast and it tends to become a god object, which is an alarm signal.
Maybe this assumption is purely based on the amount of methods available? To me, the event-store and projections are strongly coupled, and should not get decoupled. Above a gave some explanations why this is the case looking at the implementation details.
Instead of talking about implementation details, what can be done with config interop, what's part of which factories, let's talk about interfaces first.
As you guys came up with distinctions, let's define 3 "types of methods" in the event store interface (I remove the parameters for easier reading)
So this makes it a little more clear, if you want to remove all of the first 3 of the third category (createQuery(), createProjection(), createReadModelProjection()) - you also have to remove all the other ones. This leads to problems we need to think about.
Where do you want to put a method like fetchCategoryNames()? You cannot put in the the projection interface, nor in any factory.
I want to be able to send a message to a running projection and tell him to stop, so where do you want to put a method like stopProjection()?
Also if https://github.com/prooph/event-store/pull/266 gets accepted (which I would assume) - where to put methods like fetchProjectionStatus()?
Your proposal is much more than to remove 3 methods + 3 get*Factory methods, it's about 8 more methods, that do not belong to a specific projection class. I clearly understand some think that "createProjection()" does not belong to the event store interface, and the projection class has its own constructor, so this is not really needed. But there are 8 more methods you have to think about, that are not projection-constructor-related.
So before anyone invests his time into preparing some PR to remove the createProjection()-method, please think about it again and provide an example, of how the interface would look like, with all the details in mind.
codeliner
commented
7 years ago Give me two or three days. We want to use more projections in our current project. We have v7-ish mongoDB event store that throws BadMethodCallExceptions on most of the projection related methods (Third category)
So a huge amount of the new event store interface is useless for us at the moment. I'll take some time next week to investigate in the topic because I do need to find a way to get projections working with our custom mongoDB event store version. Decoupling projections from the event store could help. I'm not sure and I understand your concerns @prolic
codeliner
commented
7 years ago addition:
Maybe this assumption is purely based on the amount of methods available?
Yes, it is. You look at the event store and get the feeling that it does too much. I'm with you that it is easier for the event-store-http-api and event-store-ui to only work with a single event store instead of an event-store + x projections
But I think the latter could work, too:
$projectionManager = new ProjectionManager($redisConnection);
$categoryNames = $postgresEventStore->fetchCategoryNames();
if(!in_array('user', $categoryNames)) {
throw Mehh\NoUserCategoryAvailable();
}
$redisProjection = $projectionManager->createProjection(
'my-redis-projection',
$postgresEventStore->getStreamNamesByCategory('user'),
$myReadModel,
$projectionOptions
);
$redisProjection->runAgainst($postgresEventStore, $persistBlockSize);and to manage a running projection in a parallel process:
$projectionManager = new ProjectionManager($redisConnection);
$projectionNames = $projectionManager->fetchProjectionNames();
if(!in_array('my-redis-projection', $projectionNames)) {
throw Huh\NoRedisProjectionAvailable();
}
$status = $projectionManager->fetchProjectionStatus('my-redis-projection');
if($status === Status::ERROR) {
$projectionManager->resetProjection('my-redis-projection');
}@codeliner so your idea is to add a projection manager, right? Well okay I think I can live with that. I can provide a PR for this.
prolic
commented
7 years ago If we create a projection manager, should the fetchCategories method go there or stay in event store? Currently this is something only meaningful in projections, but on the other hand it might be useful for event store, too.
What do you think?
oqq
commented
7 years ago +1 gor the ProjectionManager. Was my first throught about stopProjection.
fetchCategories should stay in EventStore. Maybe Box a generic query method. This is the only Point in Code which is known about stored categories/streams.
The current base implementation of ProjectionOptions has some mandatory values with defaults, which would not be used in every projection class. There is also an inheritance in PdoEventStoreProjection to add more options. To provide one value other than the defaults, I have to investigate the default values in the class and add they by myself in the constructor, otherwise the object could not be build.
I guess this would escalate.
My proposal is to make this DTO more abstract, so it can be used in every current and future implementation of projection and here factories.
Default values required by the factories should be defined by the factory and not in an abstract DTO. Also, every projection should know which options are mandatory.
Here is an example implementation to be more clear what I am talking about: https://gist.github.com/oqq/c709eae6d993b49d888d15703a466f30
This would be a bc break and should not implemented in an already beta version. But I guess there is not an implementation for projections out there other than from us.
What do you think about that? @prolic @codeliner