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

Add support for reactive PubSub consumers #958

Merged
merged 21 commits into from
Oct 27, 2023
Merged

Conversation

jeremyg484
Copy link
Contributor

Support for reactive consumers is added. This gives the ability to bind the received PubSub message as a reactive type
to the subscriber method's body parameter, and to return a reactive type to which the
framework will subscribe.

Message automatic and manual ack/nack are handled accordingly.

Docs are added for the added reactive support.

This resolves issue #945

Support for reactive consumers is added. This gives the ability to bind
the received PubSub message as a reactive type to the subscriber
method's body parameter, and to return a reactive type to which the
framework will subscribe.

Message automatic and manual ack/nack are handled accordingly.

Docs are added for the added reactive support.

This resolves issue #945
@jeremyg484 jeremyg484 requested a review from sdelamo October 10, 2023 22:50
@sdelamo sdelamo requested a review from timyates October 11, 2023 14:31
@sdelamo sdelamo added the type: enhancement New feature or request label Oct 11, 2023
Copy link
Contributor

@timyates timyates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Some really nitpicky typographical suggestions (license year, avoid tabs, wildcard imports and kotlinification suggestion)

jeremyg484 and others added 10 commits October 12, 2023 06:53
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
@sonarqubecloud
Copy link

Please retry analysis of this Pull-Request directly on SonarCloud.

jeremyg484 and others added 2 commits October 12, 2023 07:00
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add tests to test-suite-groovy, test-suiteand test-suite-kotlin.

Right now the code there, which is the one shown in the docs is not tested.

jeremyg484 and others added 4 commits October 12, 2023 11:19
Co-authored-by: Sergio del Amo <sergio.delamo@softamo.com>
Tests are added to test-suite, test-suite-groovy, and test-suite-kotlin
for the relevant example code for reactive PubSub consumer support.

Tests are also added to verify the supported PubSub subscription method
message body binding scenarios, and a fix is implemented for binding
a reactive type containing an array.
@jeremyg484
Copy link
Contributor Author

@sdelamo @timyates This is ready for a re-review with the added tests. CI looks to be failing ATM because it's taking too long to download the docker image being used in the test-suite tests. I'll try to get that sorted out, but if either of you already know the potential solution to it I'd welcome your suggestions.

@sdelamo
Copy link
Contributor

sdelamo commented Oct 17, 2023

@timyates please review this again

@timyates
Copy link
Contributor

@sdelamo I am doing. I got the test-suite passing without timing out downloading the 1GB docker image

Graal is failing, and I'm not sure why...there's a discussion on Discord

Copy link
Contributor

@timyates timyates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions about excluding listeners in the suites

But thinking about it, I guess these were not executed before as there were no tests 🤔

I removed some semi-colons, and some sonar smells in a commit

@jeremyg484
Copy link
Contributor Author

Questions about excluding listeners in the suites

But thinking about it, I guess these were not executed before as there were no tests 🤔

I removed some semi-colons, and some sonar smells in a commit

@timyates Thanks for the assist! Any ideas about the Graal error that's happening now?

As for the disabled beans, you are correct that those classes were never being executed prior. In all cases I had to disable them because the PubSub topics they are trying to subscribe to either haven't been configured to exist in the docker container yet, or they overlap with the listeners that are now being tested (can't have multiple methods for the same subscription).

@timyates
Copy link
Contributor

Any ideas about the Graal error that's happening now?

There's a thread in discord, Graeme says it's being investigated

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

37.6% 37.6% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sdelamo sdelamo merged commit 160724a into master Oct 27, 2023
6 of 7 checks passed
@sdelamo sdelamo deleted the reactive-consumers branch October 27, 2023 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants