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.

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

feat: support for beanRef in KafkaListener annotation #1089

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ruskaof
Copy link

@ruskaof ruskaof commented Nov 20, 2024

this commit resolves #873

Hi, I'm new to this project so I'm not sure this is the best way to implement support for beanRef in KafkaListener annotation. Could you please review this MR? Most of the code comes from the spring kafka project https://github.com/spring-projects/spring-kafka/blob/main/spring-kafka/src/main/java/org/springframework/kafka/annotation/KafkaListenerAnnotationBeanPostProcessor.java

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Springwolf. Thanks a lot for creating your first pull request. Please check out our contributors guide and feel free to join us on discord.

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for springwolf-ui canceled.

Name Link
🔨 Latest commit 3667cef
🔍 Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/674dd46a43568e000813ef3a

Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

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

Thank you @ruskaof for looking into this issue and providing a link to the spring-kafka code. In its basic form, the change looks good to me.

I notice that due to the BindingFactory change (public interface), a lot of files needed adjustments. Additionally, Springwolf users will need to update their implementation (breaking compile change).
I wonder if it is possible to avoid this?
If not, I like to make sure that the change is future-proof for similar feature requests (method as context instead of class?). Please understand that I/we will review this change more closely due to the interface change.

I assume that tests are missing at this point, since you wanted to get a feedback about the general direction first. Before this PR can be merged, test(s) in springwolf-plugin/springwolf-kafka-plugin are required.

Also, to demo this improvement (and have another layer of test), please update the springwolf-kafka example (probably new BeanRefConsumer file).

I plan to get a second opinion from @sam0r040 tomorrow (method vs class).

In case you have any questions, let us know.
Thank you for picking up this issue!

@ruskaof
Copy link
Author

ruskaof commented Nov 22, 2024

Thanks for your review, @timonback ! I like your suggestions and I'll try to implement them. I believe that it is not possible to implement this feature without somehow passing the annotated class (or the class that declares the annotated method) to KafkaBindingFactory methods because the annotation itself does not provide any data on this.

Since one can use Kafkalister on both methods and classes, would it be right to make something like BindingContext<AnnotationType> class that contains the annotation itself and either annotated method or class and use it in the new BindingFactory methods?

@ruskaof ruskaof force-pushed the feat-bean-ref-in-kafkalistener branch from 9f32935 to 3667cef Compare December 2, 2024 15:38
…sting ones, move bean ref logic to KafkaBeanRefHelper, implement BindingContext for passing Method and Class context to factories
@ruskaof ruskaof requested a review from timonback December 2, 2024 16:09
Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

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

Hi @ruskaof,

Thank you for the update, sorry for the delay.

We like the change and breaking changes are avoided by the addition you added 👍
Also, the BindingContext encapsulates the class well.

Springwolf completely operates on classes at this point, not specific Spring bean instances. The Spring ApplicationContext is used to get the (one) current instance of a class using #getBean(clazz) [0]. The method will only return, when exactly one bean for that class is found. As a result, the following bean definition will not work:

@Bean
public Listener listener1() {
    return new Listener("topic1");
}

@Bean
public Listener listener2() {
    return new Listener("topic2");
}

We are willing to accept this at this point. The alternative is to create multiple classes extending the Listener class. Just want to make sure that you are aware of this limitation and your use-case still works?

As the getBean method may throw a NoUniqueBeanDefinitionException, can you wrap the error to indicate the class to assist the user in locating the original error? The error message may include a hint that the error is a limitation of Springwolf in combination with beanRef.

Also, in the next step, tests are required for the KafkaBeanRefHelper.

Great work! Looking forward to the addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for @KafkaListener beanRef
2 participants