GH-3807: Necessity of KafkaHandler on single method class

[chickenchickenlove]

Fixes: #3807
Issue link: #3807

From now on, the single public method without method-level @KafkaHandler, @KafkaListener in class which has class-level @KafkaListener will be registered as MethodKafkaListenerEndpoint so that it consumes messages.

Notification.

This PR will be replaced with #3870

[artembilan]

This is very close to the merge phase.
Please, consider to mention this feature in the class-level-kafkalistener.adoc and whats-new.adoc.

Thanks

[artembilan]

This is not OK.
I'm really against any blocking in the tests for nothing.
I mean this always going to add up 10 seconds to the whole test suite execution.
Now imaging we have like 3000 tests in the project and every one would contribute some delay.

I believe the configuration when we have ambiguity with several public methods has to be reject with an IllegalStateException.
No silent ignoring to make am impression that my configuration is OK.

[chickenchickenlove]

@artembilan thanks for your comments. 🙇‍♂️

This is not OK.
I'm really against any blocking in the tests for nothing.
I mean this always going to add up 10 seconds to the whole test suite execution.
Now imaging we have like 3000 tests in the project and every one would contribute some delay.

I agree.
I will remove this case from test codes.

I believe the configuration when we have ambiguity with several public methods has to be reject with an IllegalStateException.

I understand your point clearly.
However, The important thing is that the changes I made in this PR are not related to the case where there are multiple public methods.
So we don't need to consider ambiguous conditions because in that case any public method will not be registered as kafka listeners.

if (hasClassLevelListeners) {
  if (methodsWithHandler.isEmpty() && publicMethods.size() == 1 && !hasMethodLevelListeners) {
    // added this time.
  }
  else {
    // original	
  }
}

I think we would be better not to throw an error if there are several public methods when there is no @KafkaHandler method.
This is because after registering the class as a bean using @KafkaListener, there might be cases where other beans inject it and call its public methods. For example, getter methods.
If there are already users using it that way, it would be difficult to ensure backward compatibility.

@KafkaListener
class MyTakser {
  
  // This method is for @KafkaHandler implicitly.
  public void listen (String message) {
    // do something with kafka messages.
  }

  // This method is not for @KafkaHandler.
  public void getCurrentState() {
    // return current state
  }
  
  ...
}

IMHO, If we should something to do for this, I think it would be better to notify by using warning log.
For example,

The bean {BEAN_NAME} which has @KafkaListener is registered without a proper listener method.

WDYT?

[artembilan]

I think I see the picture better now. What if our class is extended and new methods appear there? Which one to choose from hierarchy then?

I am sorry to say that , but I feel more like better to leave it as is and rely only on the @KafkaHandler presence .

The issue simply could be turned into docs explaining why we require this or other annotation. We really cannot predict what purpose of the method is, so being explicit is much better, than trying to come up with other restrictions. I have totally ignored the fact that class could be extended 🤦.

And between us: I’m not a fun of annotations in Java. 😉

Still, that is my opinion on the problem of this issue, so I’m opened for other arguments to continue your effort with the code.

CC @sobychacko

[chickenchickenlove]

@artembilan
Thank you for your detailed explanation 🙇‍♂️
I prefer to use explicit one as well. annotations too, because of implicit 😉.

No worries at all — I'm happy to revert all the commits I made if needed! 😄

How about reconsidering it from this perspective?

1. In the case of a class with only a single public method

I believe we don't need to consider class extension in this case.
If the class was originally designed with a single public method intended to act as an implicit @KafkaHandler, it should be fine to assume that even if the class is extended, the same usage will be maintained.

If an extended class introduces a new public method, it will be ignored because there are two public methods.
If an extended class override parent's public method for a completely different purpose, then it should be considered the user's responsibility.

2. In the case of a class with multiple public methods

If there are multiple public methods and no explicit @KafkaHandler annotation is present, no method would be registered as a listener. it is current registering logic.

Therefore, I believe we don't need to worry about this case either.
For these reasons, I think this PR still can have value.

After you read my comments and still believe this PR is invalid, I'm happy to revert all the commits and will create commits to update class-level-kafkalistener.adoc!
Please, feel free to talk your opinion.

Thanks for your time 🙇‍♂️

[artembilan]

I think I prefer the fix in the: #3870.

since we talk about the situation when we may have the annotation on the class , but no on its methods (even if it is single ), we cannot judge what is a goal of that end-user design. May be they add method annotations in the sub-classes. So, we cannot make a decision for them and some applications might be broken if we apply this PR change.
For me it is better to document how explicit @KafkaHandler is important. Rather then make an assumption about those single methods in the class.

this is kinda similar to what we have with @RestController on the class and @RequestMapping on the methods: https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-requestmapping.html

[chickenchickenlove]

@artembilan
I got your point (especially @RestController, @RequestMapping)
I agree with your opinion. 👍
I reverted all commits and added tiny comments to class-level-kafkalistener.

When you have time, PTAL 🙇‍♂️

[artembilan]

While I'm not disagree with the info, it feels a bit awkward.
Even the first new sentence is confusing.

How about to have a text like this after that "you must specify @KafkaHandler ":

If no `@KafkaHandler` on any methods of this class or its sub-classes, the framework will reject such a configuration.
The `@KafkaHandler` annotation is required for explicit and concise purpose of the method.
Otherwise it is hard to make a decision about this or other method without extra restrictions.

[chickenchickenlove]

Nice! thanks for fixing it. 🙇‍♂️
I updated.

[artembilan]

@vitorsalves,

Please. share your opinion and we will proceed from there.
Thanks

[artembilan]

LGTM, but let’s wait for @vitorsalves approval since our solution is not what was requested.

[vitorsalves]

@chickenchickenlove @artembilan Personally, I would prefer not to have to declare @KafkaHandler in a @KafkaListener with just one method. But I understand the point about the ambiguity that this can bring and the need to have @KafkaHandler declared.

However #3870 actually helps in one of the things that bothered me the most, which was the application letting you declare @KafkaListener at the class level but not making it clear that @KafkaHandler was mandatory.

Thanks for the time and effort. I really appreciate you guys taking my idea into consideration, even if there was a better solution!