Do NOT expose Engine state if it's Append already closed due to error

[dfakhritdinov]

Motivation:

Engine.state can expose non-persisted state that should not be used by any other code. Existing implementation also cannot expose errors that prevent the state from been persisted and one outside the engine has no idea is the state persisted or speculative.

The change brings stronger guarantee of exposing actual state by raising exception that blocks the state from been persisted.

[t3hnar]

I'm not sure this is a good idea to prevent reading the state after append operation failed…

it's rather you should not do any side effects after append operation failed…

Even after adding this exception, you still have not covered case when you read state after events, however events are not applied and events append may just fail…

[dfakhritdinov]

Could you elaborate a bit more why do you prefer to expose state after persistence failure? Do you know any use cases where the state can be used safely ignoring persistence aspect?

Even after adding this exception, you still have not covered case when you read state after events, however events are not applied and events append may just fail…

Let me clarify that here you're talking about handling commands by engine, right? If so, you're right but I'd say that its separate issues that (if needed) should be addressed separately. Side effects has mechanic to verify if persistence successful of already (and forever) failed. The mechanic is optional, but AFAIK should always be used. Might be it should be enforced by engine API as well

[t3hnar]

Could you elaborate a bit more why do you prefer

I rather prefer to not get error upon reading the state.

[dfakhritdinov]

I rather prefer to not get error upon reading the state.

That's fine while will be way to know if persistence already failed. What do you think about having additional method that will return state or error?

[mr-git]

how/why having access to state when journal persistence failed is OK? isn't it something like: "we failed to persist all events, but it is OK, just keep using this state, which you will not be able to reach on next recovery"

[t3hnar]

What do you think about having additional method that will return state or error?

Will it be enough for you to have rather method that will return effective state rather than optimistic, still without error

[t3hnar]

trait Engine[F[_], S, E] {

  @deprecated("use either `effective` or `optimistic` ")
  def state: F[State[S]]

  def effective: F[State[S]]

  def optimistic: F[State[S]]
}

[dfakhritdinov]

Will it be enough for you to have rather method that will return effective state rather than optimistic, still without error

yes, that's a good alternative. Only downside I see is its implementation, afaik no effective state stored right now, only optimistic one. For that I will have to add additional Ref & logic to update it after persistence happened

[t3hnar]

Only downside I see is its implementation, afaik no effective state stored right now, only optimistic one. For that I will have to add additional Ref & logic to update it after persistence happened

I'd not call it a downside :)

[dfakhritdinov]

closed in favour of https://github.com/evolution-gaming/akka-effect/pull/223