GH-2554: Fix DEFAULT_SCHEDULER_WRAPPER_BEAN_NAME value #2555
Conversation
| @DirtiesContext | ||
| @EmbeddedKafka(topics = {RetryTopicMultipleListenerIntegrationTests.FIRST_TOPIC, | ||
| RetryTopicMultipleListenerIntegrationTests.SECOND_TOPIC}, partitions = 1) | ||
| public class RetryTopicMultipleListenerIntegrationTests { |
There was a problem hiding this comment.
I got mainly inspired by this test. Instead of adding a new test class, I can modify that class to make it cover this case or keep it as a separate class like in the PR. Or I can delete the test if you don't think it is worthwhile. Whatever you like.
There was a problem hiding this comment.
Hi @cenkakin !
Not sure why do we need such a complex integration test just to verify that there is no conflict around defaultRetryTopicKafkaTemplate bean...
Right, I see that we need a second @RetryableTopic to make it fail after gac.registerBean(RetryTopicBeanNames.DEFAULT_SCHEDULER_WRAPPER_BEAN_NAME,, but probably we indeed can cover this use-case just adding extra config into that RetryTopicSameContainerFactoryIntegrationTests you mention before.
WDYT?
Thank you for contribution!
There was a problem hiding this comment.
Yup, it is what I can do. But we need to modify Config class in RetryTopicSameContainerFactoryIntegrationTests more to trigger the line that fails.
As far as I understand from the test case intended to cover, it should be ok. Let me push another commit.
| @DirtiesContext | ||
| @EmbeddedKafka(topics = {RetryTopicMultipleListenerIntegrationTests.FIRST_TOPIC, | ||
| RetryTopicMultipleListenerIntegrationTests.SECOND_TOPIC}, partitions = 1) | ||
| public class RetryTopicMultipleListenerIntegrationTests { |
There was a problem hiding this comment.
Hi @cenkakin !
Not sure why do we need such a complex integration test just to verify that there is no conflict around defaultRetryTopicKafkaTemplate bean...
Right, I see that we need a second @RetryableTopic to make it fail after gac.registerBean(RetryTopicBeanNames.DEFAULT_SCHEDULER_WRAPPER_BEAN_NAME,, but probably we indeed can cover this use-case just adding extra config into that RetryTopicSameContainerFactoryIntegrationTests you mention before.
WDYT?
Thank you for contribution!
| } | ||
|
|
||
| @EnableKafka | ||
| @Configuration | ||
| static class Config extends RetryTopicConfigurationSupport { | ||
| static class Config { |
There was a problem hiding this comment.
We shouldn't extend RetryTopicConfigurationSupport and shouldn't inject our own ThreadPoolTaskScheduler ->
https://github.com/spring-projects/spring-kafka/pull/2555/files#diff-80d8fe2567e6640c6d1edada33ec0bf783012606c2264cf4fc6b3aed4cca9742L242
| DefaultErrorHandler errorHandler = new DefaultErrorHandler( | ||
| (cr, ex) -> latchContainer.countDownLatch2.countDown(), | ||
| new FixedBackOff(0, 2)); | ||
| factory.setCommonErrorHandler(errorHandler); |
There was a problem hiding this comment.
This is the part we no longer cover anymore.
I just pushed another commit that creates a new bean and keeps BasicKafkaListener instead of modifying it:
|
Hey @artembilan! Thank you for the feedback! I pushed commits and left comments. Basically, the following three commits present different approaches:
I would vote for the PR's current status (3rd approach). Please let me know your thoughts, and I will update the PR accordingly. |
|
Sorry, @garyrussell , some glitch has happened over here on GH, so I pressed @cenkakin , thank you for contribution; looking forward for more! |
Fixes #2554.