Add version update for ProcessManager instances#893
Conversation
Codecov Report
@@ Coverage Diff @@
## master #893 +/- ##
============================================
+ Coverage 93.22% 93.39% +0.16%
- Complexity 3823 3859 +36
============================================
Files 522 529 +7
Lines 12591 12637 +46
Branches 710 708 -2
============================================
+ Hits 11738 11802 +64
+ Misses 625 605 -20
- Partials 228 230 +2 |
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmitrykuzmin please see my comments.
| } | ||
|
|
||
| /** | ||
| * Advances the transaction version. |
There was a problem hiding this comment.
I think we should be more precise here, as transactions do not have versions themselves.
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
|
|
||
| /** | ||
| * The context which {@link EntityVersioning} uses for version increment. |
There was a problem hiding this comment.
Can we turn this value object into something that acts? E.g. can this be a VersionIncrement, which is responsible for setting a new version to an entity in a transaction?
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmitrykuzmin as discussed vocally, let's change how things work:
- Have a
Phasein a lifecycle of each entity, defining it as an atomic entity state change, that advances entity version. - Most likely,
Phasewill need to have an entity-specificVersionIncrementas a mandatory ctor param. - Looks like we are going to have at least types of
Phases:CommandDispatchingPhaseandEventDispatchingPhase. Not 100% sure, as process managers have different scenarios dispatching commands (@Assign,@Command) and events (@React,@Command), so maybe there will be operation-specific phases. - Have three types of
Transactions — one for each entity type. - Get rid of ambiguous
incrementVersion()andadvanceVersion(EventEnvelope)methods inTransaction. - Resolve the
todoitem inPmTransactionfordispatch(..)method, which is in fact never called.
| @Nullable EventEnvelope event() { | ||
| return event; | ||
| } | ||
| abstract Version nextVersion(); |
There was a problem hiding this comment.
I am not sure why this method isn't protected.
| @Override | ||
| protected void advanceVersion() { | ||
| super.advanceVersion(); | ||
| protected void incrementVersion() { |
There was a problem hiding this comment.
And let's discuss why this method got renamed.
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmitrykuzmin looks good. However, @Command cases in PM are not covered, as it looks to me.
A process manager should update its version upon commanding (either command -> one or several commands, or event -> one or several commands).
| /** | ||
| * A transaction that supports event {@linkplain EventPlayer playing}. | ||
| * | ||
| * <p>Event playing is a process of applying the events to the certain entity with no expected |
There was a problem hiding this comment.
Let's remove this sentence. It's clear from the referenced EventPlayer.
| * the type of entity ID | ||
| */ | ||
| @Internal | ||
| public class CommandDispatchingPhase<I> extends Phase<I, List<Event>> { |
There was a problem hiding this comment.
I think the problem here would be @Command-er methods in Process Manager, which return commands (split into several or transform the given one into another).
The same issue is in EventDispatchingPhase. As Process Managers may @Command upon events, turning events into commands.
|
@dmitrykuzmin let's create some tests for |
|
@armiol PTAL. The implementation does handle Commanding methods but it is indeed misleading. Refactoring it will require even more changes to how we perform |
2 participants
This PR handles issue #880.
Previously, the entity versioning didn't work for the
ProcessManagerimplementations.Now, the version of the
ProcessManageris incremented by one upon any command or event dispatched to it (as process managers can update their state in command handlers, unlike aggregates).The other side of the changes is that now
ProcessManagerstate must be consistent with the model requirements upon handling any command.For instance, if the
QuizProcmanreceives a commandPmStartQuiz, and theidfield of procman is required, the handler method should set the ID from the command right away and not wait untilPmQuizStartedevent.This is done because the version update requires the entity state to be valid.