feature: implement support for event upcasters, fix #193

[zambrovski]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[zambrovski]

The build seem to break because some issues with the build pipeline, see #194

[smcvb]

Mainly requesting changes as this code should also upcast regular event messages. There's also a discussion to be had on many-to-one and many-to-many upcaster support (or warnings).

[smcvb]

Follow-up remarks. Looking good so far though.

[zambrovski]

@smcvb I addressed your issues and believe that now the implementation is meeting the requirements. I had to tinker a little with aggregateSeq, since it is passed as long to GenericDomainEventEntry (and not as Long allowing it to be null). I believe that passing the 0L is not wrong and went for this approach. I also changed some methods to be static, if they are not operating on instance variables.

I added some documentation on private methods to reflect the state of this discussion and make it easier to understand for the further development.

[lfgcampos]

Couple of nits, mainly typos I would say but LGTM!

[smcvb]

Just one nit left, other than that, looking good @zambrovski!

[smcvb]

My concerns have been addressed, hence approving.

[smcvb]

Interesting to see your changes consistently seem to trigger the normally periodically java.lang.OutOfMemoryError: Java heap space...
I'll give the workflow a couple more tries, but if it persists, I am going to ignore it, merge it and test locally.

As I don't have a hunch where to look right away, I am taking this route in favor of a thorough fix.
If you do have an idea what might be the culprit, Simon, feel free to give your insights.

[zambrovski]

@smcvb I'm looking on it.

In general, I think it is a good idea to separate unit tests from integration tests running with Kafka ... Usually, we separate them into two phases test/integration-test (surefire/failsafe) and run using two independent Maven runs, using Maven profiles.

Such errors as here:

2021-10-29 12:40:54,247 [main] INFO  KafkaAdminUtils                [] - Completed topic deletion: testSeekUsing_ExistingToken_ConsumerStartsAtSpecificPosition_AndCanContinueReadingNewRecords
Error:  Tests run: 3, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 55.137 s <<< FAILURE! - in org.axonframework.extensions.kafka.eventhandling.consumer.streamable.TrackingTokenConsumerRebalanceListenerIntegrationTest
Error:  testSeekUsingExistingTokenConsumerStartsAtSpecificPosition  Time elapsed: 6.317 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: Consumed 0 records before timeout instead of the expected 26 records
	at org.axonframework.extensions.kafka.eventhandling.consumer.streamable.TrackingTokenConsumerRebalanceListenerIntegrationTest.testSeekUsingExistingTokenConsumerStartsAtSpecificPosition(TrackingTokenConsumerRebalanceListenerIntegrationTest.java:166)

Error:  testSeekUsingExistingTokenConsumerStartsAtSpecificPositionAndCanContinueReadingNewRecords  Time elapsed: 6.284 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: Consumed 0 records before timeout instead of the expected 26 records
	at org.axonframework.extensions.kafka.eventhandling.consumer.streamable.TrackingTokenConsumerRebalanceListenerIntegrationTest.testSeekUsingExistingTokenConsumerStartsAtSpecificPositionAndCanContinueReadingNewRecords(TrackingTokenConsumerRebalanceListenerIntegrationTest.java:205)

are probably timing / resource related issues inside the I-Test and has more to do with a problem in test setup than in implementation. It would be easier to track the problems down if those tests could be separated from the usual assertions inside JUnit tests...

[smcvb]

Thanks for your two cents @zambrovski; was leaning toward something similar for this project.
I might have time somewhere next week but can't make any promises.

[zambrovski]

I just created a new issue for that #199 ... Might find some time to make an initial PR for this...

[smcvb]

That would be amazing @zambrovski! Much appreciated.

[lfgcampos]

I am going to trust the build is correct and merge it although our workflow is not happy about it.
Thank you very much for the PR @zambrovski!