akkadotnet / akka.net

Canonical actor model implementation for .NET with local + distributed actors in C# and F#.
http://getakka.net
Other
4.69k stars 1.04k forks source link

Batching persistence journal will never emit any failed operation messages #4951

Closed Arkatufus closed 2 years ago

Arkatufus commented 3 years ago

The way it is implemented right now, batch execution is done through the ExecuteChunk method and is guarded by a circuit breaker that will emit a ChunkExecutionFailure that in turn will report a failure message to all requesters. The problem with the current implementation is that the ExecuteChunk body is guarded with a try...catch that consumes all exceptions and always returns a BatchComplete message that returns a possible cause of failure, making the circuit breaker practically useless.

Possible solutions would be:

  1. Fix the code so that the circuit breaker does it work:
    • Make sure the try...catch block re-throw the exception so that it can be caught by the circuit breaker, which then will run the proper error reporting for each requests in the batch.
    • Remove the useless and redundant Cause property from the BatchComplete message because, now, it will only fire if the batch completed successfully.
  2. Fix the batch result handler:
    • Remove the useless circuit breaker
    • Harden the BatchComplete handler by moving the ChunkExecutionFailure handler logic into the CompleteBatch method
    • Remove the now useless ChunkExecutionFailure message and handler because it will never be fired.
Aaronontheweb commented 3 years ago

Is this why https://github.com/akkadotnet/Akka.Persistence.SqlServer/issues/114#issuecomment-496951433 is still an issue possibly?

Aaronontheweb commented 3 years ago

cc @ismaelhamed

Arkatufus commented 3 years ago

yes, that is exactly the cause of that issue, because it will never be called.

Aaronontheweb commented 3 years ago

@Arkatufus this has been fixed now, correct?

Aaronontheweb commented 2 years ago

Talked with @Arkatufus during our issue review today. This was fixed this summer.