-
Notifications
You must be signed in to change notification settings - Fork 1.1k
GH-10163: Fallback to default errorChannel in RabbitStreamMessageHandler #10184
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
Conversation
Fixes: spring-projects#10163 If RabbitStreamMessageHandler was used in async mode, error messages were dropped unless a sendFailureChannel or sendFailureChannelName were provided. This adds the "errorChannel" name as a fallback. Signed-off-by: rrileyca <[email protected]>
…running in async mode so that errors are not lost. Signed-off-by: rrileyca <[email protected]>
@artembilan Third time is the charm. I hope I have cleaned up everything now up to Spring's standards! |
@@ -543,7 +544,9 @@ void testConsumeAndProduceTransaction() throws Exception { | |||
KafkaMessageDrivenChannelAdapter inbound = new KafkaMessageDrivenChannelAdapter<>(container); | |||
DirectChannel channel = new DirectChannel(); | |||
inbound.setOutputChannel(channel); | |||
DirectChannel sendFailureChannel = new DirectChannel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you introduce his variable if it is out of verification part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because without it, the call to getChannelResolver()
would throw an error because the beanFactory
is null. Instead of creating a BeanFactory and a message channel, I thought it would be harmless to just create a channel and set it in order to circumvent the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! So, we probably just can use new NullChannel()
in that setter and no any extra local variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had forgotten about the NullChannel. Good idea. Fixed.
The PR looks really good .
it is not about standards, but reasoning and convenience for all of us. The code style is also for easy to read. So, yeah, we can call it “standard”, but that came after years of cooperation with the community and back & forth attempts . Therefore now we can even call it “the way” 😉 |
Thank you for showing me the way then! This was fun. I hope to contribute more in the future - this time, with clean PRs ;) |
…lResolver() Signed-off-by: rrileyca <[email protected]>
There is one more problem , @rrileyca :
That has to be your legal name if you are OK with legal obligations according to DCO. I took a liberty to modify your final commit message on merge, but I hope you’ll fix you Git client to populate a proper name. Thank you again ! |
I had made a non-global config in the previous repo, which I since deleted and recreated. Sorry about that. My legal name is Ryan Riley, as per the Thank you! |
I think I did it right: f4670e5 |
You did! |
Fixes: #10163
If RabbitStreamMessageHandler was used in async mode, error messages were dropped unless a sendFailureChannel or sendFailureChannelName were provided. This adds the "errorChannel" name as a fallback.