Axonframework: SequenceEventStorageEngine.readEvents gives incorrect number of events from activeStorage if no events are read from historic

[haraldk]

Basic information

• Axon Framework version: 4.4.3, 4.4.5 and 4.4.6-SNAPSHOT tested
• JDK version: java 15.0.1 2020-10-20
  Java(TM) SE Runtime Environment (build 15.0.1+9-18)
  Java HotSpot(TM) 64-Bit Server VM (build 15.0.1+9-18, mixed mode, sharing)
• Complete executable reproducer if available (e.g. GitHub Repo):
  Please see
  https://github.com/AxonFramework/AxonFramework/compare/axon-4.4.x...nrkno:axon-4.4.x
  for failing test cases and proposed quick fix.

Steps to reproduce

In our production setup we are using EmbeddedEventStore, with SequenceEventStorageEngine and two JdbcEventStorageEngines, with a batch size of 150 each. This has worked fine for us for a while, but we recently enabled snapshots, and now have problems for certain aggregates.

Debugging shows that for some aggregates, we no longer receive the events after the snapshot event from AbstractEventStore.readEvents, we get a stream containing only the single snapshot event (which again causes massive problems later on, when trying to add new events). There are no exceptions or log output.

Further debugging shows we only have this problem for aggregates that have more events in the historic storage than the batch size. Increasing the batch size would "fix" the problem for all our tests. Unfortunately, we have a relatively small amount of aggregates, but many events per aggregate, so increasing the batch size to work around this issue does not work for us.

Deleting the snapshot for the aggregate from the database would also temporarily fix the problem when reading, but as soon as we try to add new events, a snapshot is created and the same problem reappears.

Another finding was that in the cases where the events were skipped, the sequence number passed to the lambda that lazy-fetch the events from activeStorage in SequenceEventStorageEngine.readEvents was always 0. We didn't investigate further at the point, but I believe there could be more hidden issues regarding this (ie. I would expect the incorrectly low number to return more or even duplicate events, not zero events).

Based on this last finding, we have created a few test cases for SequenceEventStorageEngine that will fail with the current implementation (see SequenceEventStorageEngineTest.java), and a proposed quick fix, using Math.max(seq, firstSequenceNumber) instead of seq for the first sequence number read from activeStorage.

Expected behaviour

org.axonframework.eventsourcing.eventstore.AbstractEventStore#readEvents(java.lang.String)

Should as the API docs says return

a DomainEventStream starting with the last stored snapshot of the aggregate followed by subsequent domain events.

Actual behaviour

In the cases described above, the DomainEventStream contains only a single event, the snapshot event. Subsequent domain events are skipped.

This is a problem in itself, but it also leads to other issues, like "concurrency" problems when we try to update the aggregate, as the "current version" has an incorrect sequence number due to the missing events.

---

Let us know if you need further details, we are happy to assist.

PS: I believe the proposed fix and test cases counts as trivial, but the CLA signing service at https://www.clahub.com/agreements/AxonFramework/AxonFramework does not work for me.

Best regards,

--
Harald K

[smcvb]

First of, thanks for the detailed description @haraldk. This makes understanding your scenario and the problem a lot easier.
We would actually very much value if you could follow up this issue with a pull request too, as this would enable easier reviewing from our end.

Thanks for noting the CLA signing services isn't working at the moment. I will investigate this.
In the meantime, you can still provide the PR, although it means there's no contributor agreement in place.
Whether this is a blocker for you, is something I cannot decide for you of course.

Let us know if it is, then I will come back ASAP once it is set up.
And if it isn't, then I am looking forward to your pull request!

[haraldk]

Thank you for the fast feedback!

I have created a PR #1683.

--
Harald K

[smcvb]

Thanks for creating that PR so quickly @haraldk!
As it stands I wanted to make release 4.4.6 today; now chances are high this fix can be added as well.

[smcvb]

As far as I am concerned, pull request #1683 resolved the entirety of your issue.
However, you called it a partial fix in the title. Would you mind elaborating why you noted it as a partial fix?
Otherwise, I think we are good to go to close this issue too.

[haraldk]

As mentioned in the PR #1683 discussion, I think the problem is really caused by the JdbcEventStorageEngineand/or the EventStreamSpliterator in BatchEventStorageEngine. That's what I meant by "partial".

I think the main problem is in the SQL generated that sets an upper bound on the sequence number, rather than using limit to create the correct batch size. The current implementation will still cause problems if there are somehow gaps in the sequence numbers (as the batch fetching code in the spliterator will stop if the number of results are less than the batch size).

Perhaps this should be filed as a separate issue, otherwise I suggest leaving it open until this is addressed.

Other than that, I'm happy! 😀

[haraldk]

Further testing shows that limiting the number of rows instead of the upper bound on the sequence number does indeed fix the problem (even with the original code in SequenceEventStorageEngine).

However, I don't think there's a general way to express this in SQL across different database vendors. PreparedStatement.setMaxRows(batchSize) will also work, but may not have the performance of the native SELECT TOP ..., LIMIT ... or WHERE ROWNUM <= .. variations.

Is there a way to create different SQLs for different databases in Axon?

[smcvb]

Just wanted to clarify, and keep the track record correct here too. Thanks for reaffirming.

Whenever the statement being used is incorrect for your use case, best would be to adjust the statement in your local environment.
Luckily this is relatively trivial through the JdbcEventStorageEngine's builder, where you can invoke the readEventDataForAggregate method and provide your own ReadEventDataForAggregateStatementBuilder which generates the SQL statement.

If we check the JavaDoc, specifically of the BatchingEventStorageEngine.fetchDomainEvents(String, long, int) method, I'd argue it's ambiguous whether what's done now is right or wrong.
One way or another (removing ambiguity from the JavaDoc or adjusting the SQL statements) this can be fixed, but it is in my opinion less pressing than the fix you have provided today.
Added, I'd wager the priority is not overly high either, as under normal usage (thus through the framework itself) of the EventStorageEngine the sequence number provided would have been correct.

Regardless, it's an improvement which can be made.
If you are up for it, that would be great @haraldk.

When it comes to this issue though, I regard it as resolved with the PR you have provided.
As such, I will be closing it as well.
Feel free to keep commenting here if any further information is required.

[smcvb]

Ha, missed your previous comment there when I sent my previous comment.
I was already under the assumption that the desired solution you were looking for might be vendor-specific.

That is the exact reason why the JdbcEventStorageEngine provides the possibility to change statements through its builder.

In a future situation, it would be great if we could have a statement provider per database vendor.
I however do not see us implement something like that in the foreseeable future.
Partially because it is relatively straightforward to adjust the statements yourself, but also partially because of Axon Server.