Review PersistentEntity API for reply and afterPersist

[octonato]

This issue is a starting point for a broader discussion we would like to initiate about the current state of PersistentEntity API.

This was initially triggered by PR #901. The main problem reported there is that it's very cumbersome to write command handlers that reply with the entity state while emitting many or none events.

For illustration:
(examples in Java syntax)

final List<Event> createdEvents = generateEvents();
if (createdEvents.size() == 0) {
  ctx.reply(state().getModel());
  return ctx.done();
} else {
   return ctx.thenPersistAll(
                   createdEvents, 
                    () -> {  ctx.reply(state().getModel()); }
              );
}

In its current form, the API offers no easy way to deal with the fact that events may not be emitted.

The ctx.thenPersistAll method receive an empty lists of events, but the afterPersist method is not called if there are zero events persisted. Which make sense, because it's called afterPersist.

The mechanism is based on aContext that is passed to the user. The Context brings to user space (without exposing it) the actor.sender(). In the case of reply, behind the scene the reply calls sender() ! someMessage.

Note that we are basically piggybacking the afterPersist method to call indirectly the actor.sender() method.

Fluent API Proposal

We could achieve exactly the same without relying on a context and without mixing side-effect function (afterPersist) with reply functions.

For instance...

PersistAll(generateEvents())
  .replyWith( state -> state.getVersion(); );

The above example replies by returning the version of model. The state is the model after applying the generated events. If generatedEvents return a empty list, state is not updated.

Note that this API removes completely the need for a Context. We just register a function State -> A that is called to transform the current state and generate the data that will be sent back to the sender.

Another variation could be:

PersistAll(generateEvents())
  .replyWith( (state, events) -> state.getVersion(); );

In that case, the user receives the current updated state and the generated events.

A similar fluent API can be implement for afterPersist.

PersistAll(generateEvents())
  .afterPersist( events -> // do some side-effect here ) // <- symmetric api, see below
  .replyWith( state -> state.getVersion(); );

afterPersist callback is only called if events.nonEmpty and it returns void or Unit (scala).

PersistOne / PersistAll asymmetry

The afterPersist callbacks for PersistOne and PersistAll are not symmetric.

PersistOne has Event -> void and PersistAll has () -> void.

The reason for that comes from akka-persistence itself where the handler for persistAll has the same shape (Event -> void) as the handler for persist. This is probably for historical reasons, but as a consequence the Lagom API exposed it as Event -> void and () -> void.

Some users have complained on Gitter about it. There is a easy workaround which is to have the generated events in scope when defining the command handler. However, we can easily improve the user experience by offering a symmetric API.

[bozzzzo]

I like it!

[guizmaii]

As a first observation,

PersistAll(generateEvents()).replyWith( (state, events) -> state.getVersion(); );

seems better than

PersistAll(generateEvents()).replyWith( state -> state.getVersion(); );

[octonato]

@guizmaii we probably want the same for the afterPersist

PersistAll(generateEvents())
  .afterPersist( (state, events) -> // do some side-effect here )
  .replyWith( (state, events) -> state.getVersion(); )

[jroper]

I like these suggestions. A few questions:

• We could make the API enforce that you can only reply once (by making replyWith a method on Replyable, and ensuring PersistAll(...) returns a Replyable but ensuring replyWith doesn't return a Replyable). Should we do that?
• We could also make the API enforce that all commands are replied to (by making replyWith return a DirectiveWithReply and requiring that all command handlers return a DirectiveWithReply). Should we do that?
• Is there a defined ordering for afterPersist and replyWith callbacks? Does it matter what order they are specified?
• Can you supply two afterPersist effects?
• What happens if either an afterPersist or a replyWith callback throws an exception? Does that prevent other callbacks from being invoked?
• Can/should afterPersist be an asynchronous operation that delays the reply from being sent until it completes?

[octonato]

We could make the API enforce that you can only reply once (by making replyWith a method on Replyable, and ensuring PersistAll(...) returns a Replyable but ensuring replyWith doesn't return a Replyable). Should we do that?

For sure. I didn't thought about it initially but that's certainly a good idea.

We could also make the API enforce that all commands are replied to (by making replyWith return a DirectiveWithReply and requiring that all command handlers return a DirectiveWithReply). Should we do that?

The replyWith method would play the role of a build method in a builder which is not bad, but sometimes a user simply wants to declare what must happen as have a default reply (Done).

Is there a defined ordering for afterPersist and replyWith callbacks? Does it matter what order they are specified?

I think the API should enforce it. A Replyable would help here. After calling replyWith nothing else can be declared.

Can you supply two afterPersist effects?

Yes, why not. I would prefer to declare two afterPersist doing two different things than one mixing two different operations.

What happens if either an afterPersist or a replyWith callback throws an exception? Does that prevent other callbacks from being invoked?

Exception in replyWith would mean caller get a failure. replyWith callback only happens after afterPersist and would have no impact on them.

Not sure about exception in afterPersist. I see afterPersist as a best-effort call. If it fails, doesn't change the fact that the command was accepted and events generated. From the entity perspective, afterPersist should not be relevant. It's really for side-effecting and as such I think it should not impact the rest.

Can/should afterPersist be an asynchronous operation that delays the reply from being sent until it completes?

Not sure, the fact that it could influence replyWith means that we need to inform the caller that the command was accepted, but something else failed. Simply retuning a failure would be confusion because a re-try could:

• fail because entity has already another state
• succeed, but produce no events (idempotent). In which case, the afterPersist won't be called again making re-try useless

The only valid case would be a re-try with new events generated.

[jroper]

The replyWith method would play the role of a build method in a builder which is not bad, but sometimes a user simply wants to declare what must happen as have a default reply (Done).

If the reply type is anything other than Done, then there can be no default reply. We may still be able to implement a default reply in Scala by providing an implicit conversion from Replyable[Done] to DirectiveWithReply (or whatever we end up calling it), though there could be some challenges in the Scala compiler that make that difficult to work. The Java API won't be able to implement anything like that though, the best it could do is just reply with Done and let the caller get a ClassCastException.

[olivierdeckers]

This would be an improvement. I like having the updated state (after applying the event handlers) in the reply method, because I often find myself having to repeat the same transformation of the event handler in the afterPersist.

I agree that afterPersist should be a best-effort call. We perform side effects for which this is not enough using topics and the message broker.

This change would break code using the current api. What would the migration path look like?

[octonato]

@olivierdeckers, I think the best thing to do in this case is to provide an alternative API living next to the current. That alternative API would be marked as experimental and we would like to, guess what, experiment with it and have feedback from the community.

Once we decide about it's shape we can make it the default one and deprecate the old.

[octonato]

We have published a first stab for a new Lagom Persistence API.
https://github.com/lagom/persistence-api-experiments

This is only a sandbox that we are using to design the API and get the types on the right place. It's only Scala for the moment and it even don't use Akka yet.

Feedback is more than welcome.

[mihbor]

I really like the explicit handling of entity creation. How would deletion be handled? By an event-handler that sets the state to None or an explicit API as well?

[octonato]

@mihbor I have discussed this topic a couple of times. The most common argument against it is that an Entity in an Event Sourced app is never deleted. I don't agree with it. What is never deleted are the events. The Entity itself is just an accumulation of them and that doesn't mean that you can't have an event that removes the accumulation.

I have seen a system that did something on those lines. The Entity was a kind of shopping cart, the ID was the user id. It got filled and on "Order" or "Cancellation" it got deleted. That was their to empty the cart when it was not needed anymore (instead of removing item per item).

This is the equivalent of a hard delete in a CRUD app. After hard deleting something, you can recreated it using the same ID. The drawback of it in a Event Sourced app is that you keep the history. Two incarnations of for the same ID won't have distinct histories, but they will share one and single history.

For that reason, an not the fact that Entity are not deletable, I would refrain to add a delete option.

[octonato]

For clarity, I'm not saying that I'm totally against a delete. It's just something that we should be careful. If we ever add it we need to have a very good case for it and a good companion article / blog explaining what that really means to 'delete' an Entity.

And uninformed users will probably be trapped and believe that such a method would delete the events.

[mihbor]

Sure, I get it. Perhaps I should have elaborated a bit more. What I meant was that, now that there is going to be explicit handling for an initial "non-existence" of entities, what will be the best way to handle a (deletion) event to transition the entity state to be equivalent to that of a non-existent entity for the purpose of handling further commands. Currently I just set the state to null and set the behaviour to the same one as when the initialBehavior method is called with an empty snapshot.
Will it stay the same? That is, if I set state to null and if the entity gets snapshotted with a null state and passivated and then restarted from that empty snapshot, will that trigger "Behavior.first", as in:
Behavior .first { // () => Actions equivalent to None => Actions
just as if it never existed?

NB. just having written the above question does make me think having an explicit deletion would make it clearer rather than more confusing, but that's just my personal opinion.

[octonato]

When the State becomes Option[State], applying an Event will be.

def applyEvent(state: State, evt: Event): State 

stateOpt.map { state => applyEvent(state, event) } 

If an event handler returns null you will end up with Some(null), instead of None.

We could fix that by adding special delete handlers or by converting all Some(null) automatically to None.

stateOpt
  .map { state => applyEvent(state, event) } 
  .filter(_ != null)

That will bring your model back to None and on next command you will be using Behavior.first once again.

I have to say that I'm not comfortable with the idea of using null to express deletion, but I understand that was the only way to do it before.

The alternative would be:

Behavior
  .first { } 
  .andThen { } 
  .reset { } // reset may be a better name to avoid confusion with events deletions. 

[mihbor]

Yes, I'd too prefer to avoid the null in the new API.

[octonato]

This becomes obsolete with the upcoming Akka Persistence Typed API

Thanks for all the thoughts and feedback.