spring-projects / spring-kafka

Provides Familiar Spring Abstractions for Apache Kafka
https://projects.spring.io/spring-kafka
Apache License 2.0
2.19k stars 1.56k forks source link

Docs say `defaultRetryTopicKafkaTemplate` is the default template bean name. `@RetryableTopic` javadoc says `retryTopicDefaultKafkaTemplate` #3514

Closed ndwhelan closed 1 month ago

ndwhelan commented 1 month ago

In what version(s) of Spring for Apache Kafka are you seeing this issue?

Between (at least) 2.9.13 and 3.2.4

Describe the bug

The documentation states that the default KafkaTemplate bean for publishing retries / DLT through @RetryableTopic is defaultRetryTopicKafkaTemplate. The annotation states, and at least some tests use this, that it should be retryTopicDefaultKafkaTemplate. On the other hand, some tests and code refer to defaultRetryTopicKafkaTemplate through RetryTopicBeanNames.DEFAULT_KAFKA_TEMPLATE_BEAN_NAME

I don't know which one to use at this point. Though I'm working around this by specifying a specific values for the kafkaTemplate parameter on the @RetryableTopic annotation.

To Reproduce

To be honest, I'm having trouble determining which bean its using.

Expected behavior

I would expect consistency in the javadoc and the docs on the Spring Kafka website to be consistent. At this point, I don't know which one is actually used in the code.

Sample

If necessary, I will work on an isolated example in a new GitHub repository. Though, i'm hoping this issue/concern is straightforward enough, with the links above, that it's not necessary. Still, I understand if it is.

artembilan commented 1 month ago

That JavaDoc has to be fixed. The logic there in the RetryableTopicAnnotationProcessor is like this:

    private KafkaOperations<?, ?> getKafkaTemplate(@Nullable String kafkaTemplateName, String[] topics) {
        if (StringUtils.hasText(kafkaTemplateName)) {
            Assert.state(this.beanFactory != null, "BeanFactory must be set to obtain kafka template by bean name");
            try {
                return this.beanFactory.getBean(kafkaTemplateName, KafkaOperations.class);
            }
            catch (NoSuchBeanDefinitionException ex) {
                throw new BeanInitializationException("Could not register Kafka listener endpoint for topics "
                        + Arrays.asList(topics) + ", no " + KafkaOperations.class.getSimpleName()
                        + " with id '" + kafkaTemplateName + "' was found in the application context", ex);
            }
        }
        Assert.state(this.beanFactory != null, "BeanFactory must be set to obtain kafka template by default bean name");
        try {
            return this.beanFactory.getBean(RetryTopicBeanNames.DEFAULT_KAFKA_TEMPLATE_BEAN_NAME,
                    KafkaOperations.class);
        }
        catch (NoSuchBeanDefinitionException ex2) {
            KafkaOperations<?, ?> kafkaOps = this.beanFactory.getBeanProvider(KafkaOperations.class).getIfUnique();
            Assert.state(kafkaOps != null, () -> "A single KafkaTemplate bean could not be found in the context; "
                    + " a single instance must exist, or one specifically named "
                    + RetryTopicBeanNames.DEFAULT_KAFKA_TEMPLATE_BEAN_NAME);
            return kafkaOps;
        }
    }

So, t definitely uses the mentioned RetryTopicBeanNames.DEFAULT_KAFKA_TEMPLATE_BEAN_NAME constant. There the JavaDoc is misleading and has to refer to this constant as well.

Contribution is welcome!

ndwhelan commented 1 month ago

Thanks for quickly triaging and providing this information!

I may be able to contribute the necessary changes this weekend or next week... we'll see. Hopefully.

sobychacko commented 1 month ago

@ndwhelan Are you still planning to contribute these changes? Thanks!

JeonDaehong commented 1 month ago

@ndwhelan @artembilan

https://github.com/spring-projects/spring-kafka/pull/3543

Hello, I have made the changes and submitted a PR. This is my first open-source PR. I would be grateful if you could accept the PR and give me the opportunity to become a contributor. Thank you!

ndwhelan commented 1 month ago

Thanks for taking this! I hadn't been able to get to it.

I'm outside the org here, though, and can't merge.