Skip to content

Add auto-configuration for RabbitStreamTemplate #28060

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
Closed
wants to merge 2 commits into from
Closed

Add auto-configuration for RabbitStreamTemplate #28060

wants to merge 2 commits into from

Conversation

eddumelendez
Copy link
Contributor

RabbitStreamTemplate is provided at spring-amqp 2.4.

@eddumelendez
Add RabbitStreamTemplate configuration
96300a3 
`RabbitStreamTemplate` is provided at spring-amqp 2.4.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 17, 2021
@snicoll
Copy link
Member

snicoll commented Sep 21, 2021

paging @garyrussell for a review.

snicoll
snicoll reviewed Sep 21, 2021
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I don't know this feature but I've added a few observations.

...igure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java Outdated
@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(RabbitStreamTemplate.class)
@ConditionalOnMissingBean(RabbitStreamTemplate.class)
@ConditionalOnProperty(prefix = "spring.rabbitmq.listener", name = "type", havingValue = "stream")
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I got that. Isn't the listener unrelated to the creation of the template? The latter is for programmatic access, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - using a stream producer is independent of whether listeners are also stream, by default.

Also, for consistency, this should be moved to RabbitStreamConfiguration (which will need some rework because the whole class is excluded via this property - move the condition from the class to the factory @Bean.

The Environment @Bean is needed for both producers and consumers.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Sep 21, 2021
garyrussell
garyrussell suggested changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

Sorry for the delay - I was on PTO.

...igure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java Outdated
@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(RabbitStreamTemplate.class)
@ConditionalOnMissingBean(RabbitStreamTemplate.class)
@ConditionalOnProperty(prefix = "spring.rabbitmq.listener", name = "type", havingValue = "stream")
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - using a stream producer is independent of whether listeners are also stream, by default.

Also, for consistency, this should be moved to RabbitStreamConfiguration (which will need some rework because the whole class is excluded via this property - move the condition from the class to the factory @Bean.

The Environment @Bean is needed for both producers and consumers.

@snicoll
Copy link
Member

snicoll commented Oct 1, 2021

@eddumelendez would you have time to review the PR?

@eddumelendez
Copy link
Contributor Author

@snicoll I will work on the changes next week

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 1, 2021
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 21, 2021
@snicoll
Copy link
Member

snicoll commented Nov 8, 2021

@eddumelendez would you have time to look at the other comments in the review? I am not sure they were all addressed. If you don't, just let us know and we can take over. Thanks!

@eddumelendez
Copy link
Contributor Author

@snicoll I will be updating the PR in the next couple of days. sorry for the delay.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 9, 2021
@snicoll
Copy link
Member

snicoll commented Nov 9, 2021

No worries, it is flagged for 2.7.x so you have time.

@eddumelendez eddumelendez requested a review from snicoll November 10, 2021 03:58
@philwebb philwebb force-pushed the main branch 3 times, most recently from 1ca278f to 902dd0b Compare November 19, 2021 20:17
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Dec 10, 2021
@snicoll snicoll added this to the 2.7.x milestone Dec 10, 2021
@snicoll snicoll changed the title Add RabbitStreamTemplate configuration Add auto-configuration for RabbitStreamTemplate Jan 3, 2022
@snicoll snicoll self-assigned this Jan 3, 2022
@snicoll snicoll modified the milestones: 2.7.x, 2.7.0-M1 Jan 3, 2022
snicoll pushed a commit that referenced this pull request Jan 3, 2022
@eddumelendez @snicoll
Add support for RabbitStreamTemplate
3952046 
See gh-28060
snicoll added a commit that referenced this pull request Jan 3, 2022
@snicoll
Polish "Add support for RabbitStreamTemplate"
6b6da22 
See gh-28060
@snicoll snicoll closed this in 3d5ea71 Jan 3, 2022
@snicoll
Copy link
Member

snicoll commented Jan 3, 2022

Thanks again @eddumelendez

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell requested changes

@snicoll snicoll Awaiting requested review from snicoll

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.7.0-M1
Development

Successfully merging this pull request may close these issues.
5 participants
@eddumelendez @snicoll @garyrussell @wilkinsona @spring-projects-issues