Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native Kotlin suspending gateways #107

Conversation

jnfeinstein
Copy link

@jnfeinstein jnfeinstein commented Mar 9, 2021

This PR provides asynchronous gateway implementations relying on Kotlin coroutines. It is largely a refactor of the ReactorCommandGateway found in extension-reactor, but with the guts swapped out. I believe this implementation will integrate easier with Kotlin projects as it does not require knowledge or usage of Project Reactor. Behavior should mimic that of CommandGateway when it is not possible to match ReactorCommandGateway due to Reactor specifics.

I have dropped support for running multiple commands and queries. These are implemented in extension-reactor using flatMap syntax, the equivalent of which would be simple loops in Kotlin. I have also replaced queries using Class syntax (as opposed to ResponseType syntax) with inline reified generic implementations.

@CLAassistant
Copy link

CLAassistant commented Mar 9, 2021

CLA assistant check
All committers have signed the CLA.

@jnfeinstein
Copy link
Author

@sandjelkovic I think the checks are failing with permissions errors. Tests pass locally.

@jnfeinstein jnfeinstein force-pushed the task/suspending-command-gateway branch 2 times, most recently from d5e57d5 to fba6d5d Compare March 10, 2021 14:57
@jnfeinstein jnfeinstein changed the title Native kotlin suspending command gateway Native kotlin suspending command and query gateways Mar 11, 2021
@jnfeinstein jnfeinstein force-pushed the task/suspending-command-gateway branch from c4f25f7 to ca71397 Compare March 11, 2021 20:27
@jnfeinstein jnfeinstein marked this pull request as draft March 11, 2021 21:11
@jnfeinstein jnfeinstein force-pushed the task/suspending-command-gateway branch 2 times, most recently from afe06ba to 16c40e6 Compare March 11, 2021 21:27
@jnfeinstein jnfeinstein marked this pull request as ready for review March 11, 2021 21:27
@jnfeinstein jnfeinstein changed the title Native kotlin suspending command and query gateways Native Kotlin suspending gateways Mar 11, 2021
@jnfeinstein jnfeinstein force-pushed the task/suspending-command-gateway branch from 16c40e6 to 867afe7 Compare March 16, 2021 14:01
@sonarcloud
Copy link

sonarcloud bot commented Mar 16, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@smcvb
Copy link
Member

smcvb commented Apr 15, 2021

Our apologies for the longer than usual wait here @jnfeinstein.
Between me and @sandjelkovic we actually had a little bit too much on our plate the last couple of weeks.
However, I'm responding here because today we did find the time to chat about this PR of yours, which we felt was required to do your idea justice.

Firstly, we're keen to learn what your driving force was to provide this pull request, Joel.
I do get the grasp you've shared in the header of this PR, but if there are any points you are able to elaborate on that would be fantastic.
This request stems from the fact that, in essence, a lot of these operations can just as well be achieved with the await method you can invoke on results of the respective gateways.
However, maybe we're missing some obvious benefits in this area, which we hope you can fill in for us.

Secondly, I have a heads-up for you on a potential other solution we were thinking of.
What if the Kotlin Extension utilizes the Reactor Extension instead, and provides simple extension methods to comply with Kotlin's coroutine approach?
I'd wager this merits the introduction of a kotlin-reactor module (naming not fixed of course) in this project, so that users are free to use/not use this solution.

Furthermore, this would lose the conceptual copy you've done here.
The downside would be that the Reactor Extension would become a requirement to utilize these methods, of course.
Whether that's the right cut-off to make is still under debate over at AxonIQ though.
If you however have the time to give such a solution a spin, granted you see a benefit in it, that would be very much appreciated of course.

Anyhow, let us know what your thoughts are on the above.
And, thanks for providing this contribution of course!

@smcvb
Copy link
Member

smcvb commented Oct 12, 2021

Due to inactivity, this issue is being closed.

If time allows it, @jnfeinstein, please look at the earlier asked question from my end.
That might just turn into me opening this issue again.

@smcvb smcvb closed this Oct 12, 2021
@jnfeinstein
Copy link
Author

jnfeinstein commented Oct 12, 2021

Heh, thanks @smcvb. Four months should've been more than ample time for my to draft a response. 😆

It's been a few months so please excuse a few cache misses in my memory. Reactor is excellent software, and can absolutely handle the use-case of asynchronous command/query/event handling. It does however require additional dependencies both in terms of code and knowledge, and the learning curve can be steep.

To be frank, it really wasn't such a lift to write suspending gateway implementations, especially with the extension-reactor code as a baseline. The bus implementations get you 95% of the way there, and the rest is a few Kotlin details.

Stretching my memory a bit here - I think that the underlying threading infrastructure is also different. The Kotlin suspending infrastructure uses coroutine dispatchers, whereas Reactor uses the netty event loop, which are not the same. Kotlin authors will find it easier to stay within the single world of coroutine dispatchers rather than having to bridge two threading models. Plus lower cost in system resources.

Finally, I'd like to see the Kotlin extension more closely aligned with the patterns of extension-reactor (kudos to @m1l4n54v1c for some great code there). No offense to the authors of extension-kotlin, but I haven't seen the callback pattern used elsewhere in the Kotlin ecosystem. Providing complex core components (gateways, response types, etc) that really bring Axon closer to Kotlin would be of great use to future authors.

@smcvb
Copy link
Member

smcvb commented Oct 15, 2021

Thanks for the insights here, @jnfeinstein. And, no worries for the late reply. We're sometimes also richly late because of other obligations 😅

If I follow your suggestions closely, I believe you're OK with keeping this issue closed(?), as long as we move the Kotlin Extension to provide more core component support, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants