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.

Sign up for GitHub

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

Jump to bottom

Add zip-split-rabbit-binder sample #108

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Add zip-split-rabbit-binder sample #108

merged 2 commits into from
Dec 9, 2024

Conversation

artembilan
Copy link
Member

@artembilan artembilan added the type: enhancement A general enhancement label Dec 6, 2024
@artembilan artembilan added this to the 5.0.2 milestone Dec 6, 2024
onobc
onobc requested changes Dec 8, 2024
View reviewed changes
Copy link
Collaborator

@onobc onobc left a comment

Choose a reason for hiding this comment

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

Nice sample @artembilan! I ❤️ how little code it took to get the pipeline put together. Nice work.

I have added some comments/suggestions PTAL.

samples/zip-split-rabbit-binder/README.adoc Outdated Show resolved Hide resolved
samples/zip-split-rabbit-binder/README.adoc Outdated Show resolved Hide resolved
samples/zip-split-rabbit-binder/build.gradle Show resolved Hide resolved
.../zip-split-rabbit-binder/src/test/java/com/example/ZipSplitRabbitBinderApplicationTests.java Outdated Show resolved Hide resolved
.../zip-split-rabbit-binder/src/test/java/com/example/ZipSplitRabbitBinderApplicationTests.java Outdated Show resolved Hide resolved
.../zip-split-rabbit-binder/src/test/java/com/example/ZipSplitRabbitBinderApplicationTests.java Outdated Show resolved Hide resolved
.../zip-split-rabbit-binder/src/test/java/com/example/ZipSplitRabbitBinderApplicationTests.java Outdated Show resolved Hide resolved
.../zip-split-rabbit-binder/src/test/java/com/example/ZipSplitRabbitBinderApplicationTests.java Show resolved Hide resolved
samples/zip-split-rabbit-binder/README.adoc Outdated Show resolved Hide resolved
samples/zip-split-rabbit-binder/README.adoc Show resolved Hide resolved
@artembilan
Address PR reviews:
01d22ed 
* Fix typos and language in README
* Fix new line in the end of `application.yml`
* Move `@Bean` for `RabbitMQContainer` as a regular `static` property on the class with a `@Container`
* Use `@Testcontainers(disabledWithoutDocker = true)`
* Move `@RabbitListener` method directly to test class
* In the end we don't need extra `@TestConfiguration` class at all
@artembilan artembilan requested a review from onobc December 9, 2024 13:59
@artembilan
Copy link
Member Author

Thanks, @onobc , for review!

I ❤️ how little code it took to get the pipeline put together.

Well, that took me a couple days to figure out all the moving parts from Spring Cloud Function and Spring Cloud Stream how to make it working together.
This splitter-to-splitter task gave me a lot of headaches.
Will chat to Oleg if that is possible to make it easier, as I believe users of our libraries would straggle as well 😢

@onobc
Copy link
Collaborator

onobc commented Dec 9, 2024

Thanks, @onobc , for review!

You are more than welcome @artembilan

I ❤️ how little code it took to get the pipeline put together.

Well, that took me a couple days to figure out all the moving parts from Spring Cloud Function and Spring Cloud Stream how to make it working together. This splitter-to-splitter task gave me a lot of headaches. Will chat to Oleg if that is possible to make it easier, as I believe users of our libraries would straggle as well 😢

This is exactly why it is a perfect candidate for a sample. I too would have struggled how to do this. I was amazed how little actual code it did take once you figured out all the moving pieces. If we could make it easier for users to figure out that would be amazing.

onobc
onobc approved these changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@onobc onobc left a comment

Choose a reason for hiding this comment

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

Looks great @artembilan

@artembilan
Copy link
Member Author

Had discussion with Oleg: it cannot be easier, since function is one-to-one, therefore it is really expected that List<?> output is a single message as an input for the next.
So, this example is a nice show-case how reactive types make things working since we interact only with a single input Flux and single output Flux: the function contract is not broken.
It is now up to end-user to decide that one item of the Flux has to be splitted - in reactive terms flat-mapped.
And that's why our out-of-the-box splitterFunction has to become reactive.

@artembilan
Copy link
Member Author

@corneil , @sobychacko ,

are you going to look into this?
Or just @onobc 's review is enough?

What is the policy then? Should I merge myself?

Thanks

@onobc
Copy link
Collaborator

onobc commented Dec 9, 2024

@corneil , @sobychacko ,

are you going to look into this? Or just @onobc 's review is enough?

What is the policy then? Should I merge myself?

Thanks

I would say 1 review is fine and go ahead and merge it.

@artembilan artembilan merged commit 0338922 into main Dec 9, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onobc onobc onobc approved these changes

@corneil corneil Awaiting requested review from corneil

@sobychacko sobychacko Awaiting requested review from sobychacko

Assignees
No one assigned
Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
5.0.2
Development

Successfully merging this pull request may close these issues.
2 participants
@artembilan @onobc