Get rid of original events in Aggregate/FunctionalCommandService

[ugumba]

It should not be the Aggregate's or FunctionalCommandService's responsibility to expose an exhaustive event history to consumers. This won't scale. Either the state captures a synopsis of the required event information, or the event stream is projected to a separate read model fit for the consumer's purpose.

More to the point, the "Original" event array blocked adding support for loading from a cached snapshot + stream tail (see upcoming PRs). Maintain OriginalVersion instead.

[CLAassistant]

All committers have signed the CLA.

[alexeyzimarev]

I am not willing to remove the original events collection. There must be another way to do it. I would recommend opening an issue to discuss the proposed changes before implementing it. There's (usually) a reason for certain things to be in the code base, and taking things out because some use case doesn't fit is not a good idea. There must be a way to find balance between the use cases.

[ugumba]

There's (usually) a reason for certain things to be in the code base, and taking things out because some use case doesn't fit is not a good idea.

I understand completely. I'll see if I can find a way to satisfy both - in my mind, having all events as baggage on aggregate was orthogonal to the whole idea of event sourcing. I could only see a few tests which made use of them in this repo, so I took too much of a liberty.

BTW, these PRs were meant to be discussion vehicles with running code, not final submissions.

[alexeyzimarev]

Cool, thanks for clarifying.

I don't think having the list of events is in any way contradicting the idea of event sourcing. Basically, events represent state transitions and there's no explicit need to fold them into a state object if the state can be determined by looking at events. I discussed it with Jeremie last year, that's where the idea comes from. Here's the article, and the direct link to "fold or not": https://thinkbeforecoding.com/post/2021/12/17/functional-event-sourcing-decider#Or-not-fold

But, indeed, we can find a middle ground as having the original events collection in the aggregate could be optional. I am having some time off this week, it gives me some time to think about it.

[ugumba]

Sure, I understand that it may have merit to use the event history as state in scenarios where typical aggregate history is negligible, or the number of "hot" aggregates is low.

But comparing to "the vast majority of ES systems", and considering scenarios with millions of events per aggregate (which I think is perfectly valid), I think it's wrong to make the event history a compulsory part of the core Aggregate type (or "folded event stream").

Caching, persisted snapshots and incremental read models should ensure that only new events are iterated - reading and iterating (and even loading into RAM) the full stream must be considered costly and prohibitive.

I have no problem with the event history being optional in Aggregate: e.g. configurable by the aggregate factory, added via a specialized Aggregate type, or provided via callbacks - anything but compulsory 😃.

[alexeyzimarev]

Yeah, it makes sense, although I don't think that an aggregate with millions of events is something that could be considered "normal". I've seen cases like that, and it was always something weird with the model.

Regardless, I agree that it should be made optional.

[alexeyzimarev]

Actually, the issue with OriginalEvents is not existent as it won't be populated with anything more than the events (including snapshots) from the store. So, if snapshots are there and only one snapshot and 10 events are loaded, the collection will contain 11 elements. It won't have more than required.

[ugumba]

Sorry - I've have to make progress with my application, and have decided to not use Eventuous - I just can't find the time to achieve the behaviour I need while ensuring backwards compatibility.
Unless told not to, I'll close these PRs soon.

[alexeyzimarev]

Sure, no problem. As I said, the claim of the PR that having the events collection won't scale isn't exactly right as it should contain only what's physically loaded for restoring state. It should not contain more elements than what's loaded from the database: events, or snapshot + events. So, I believe this PR can be closed.