Closed guizmaii closed 4 years ago
Looks good in general. A couple of observations:
CommandAPISet
class, which in this class replaces the getCommandAPISet
with a createCommandAPI
method (i.e. for a single aggregate). See #39start
, so the result of the start
(which in this PR would be the EventSourceApp
, could cease to exist or be void
. In which case the EventSourcedAppBuilder
can just be a builder but doesn't need to be renamed, can just remain EventSourcedApp
.A very much analogous setup is the SagaApp in simple sagas:
You add actions instead of commands. See:
https://simplesource.io/simple_sagas_saga_coordinator.html
https://github.com/simplesourcing/simplesagas/blob/master/simplesaga-saga/src/main/java/io/simplesource/saga/saga/app/SagaApp.java
This (i.e. the run
method) just returns void
, and the client is completely separate.
Digression alert...!!:
What works really nicely there is the PropertiesBuilder.BuildSteps propertiesBuildSteps
parameters - it gives greater control to how the
properties are set for streams, producers and admin client etc. The framework sets reasonable defaults, but the user has complete control over these, whereas in simple sourcing we kind of prescriptively end up overriding certain properties in these cases.
!!!Digression over - apologies!
Hey @szoio ,
I have an existing PR to remove the CommandAPISet class, which in this class replaces the getCommandAPISet with a createCommandAPI method (i.e. for a single aggregate).
Yes, I saw it. I'm interested. This would simplify the benchmark lib code.
For the rest of your remark, I don't know.
It seems to be a lot of changes, so a certain amount of work that I don't personally have the time to do. Plus, these changes will go against what I've already done for the benchs, requiring me to rewrite a certain amount of what I've done. Since I still have a lot of work on the benchs side, I would prefer to not change too many things here right now in SimpleSource and just continue my work on the benchs.
When the changes you describe here are implemented, it'll be time to adapt the benchs lib.
WDYT?
And in general I'm not a big fan of functions/methods returning void
or Unit
😉
Two modifications here:
EventSourcedApp
. It should help to prevent the user to call thestart
method more than once inadvertently.addAggregate(AggregateBuilder => Unit)
by make it more type safe. It's nowaddAggregate(AggregateBuilder => AggregateSpec)
This
EventSourcedAppBuilder
helps me to know if I'm manipulating an already started app or just a description of an app.For example, in the benchs, here's the app "description" site: https://github.com/guizmaii/simple-benchs/tree/65ee622896fe4a50633f18f145c9b4a18f9c434f/benchs/src/it/scala/io/simplesource/benchs/it/BenchConfigs.scala#L32
It's where the user of the benchmark lib I'm writing is declaring what is the app he want to bench.
And here's the launch site: https://github.com/guizmaii/simple-benchs/tree/65ee622896fe4a50633f18f145c9b4a18f9c434f/benchs/src/main/scala/io/simplesource/benchs/gatling/protocol/Predef.scala#L100, which is part of the internals of the lib I'm writing, not something the user need to see nor handle.
LMKWYT 🙂