chickenchickenlove commented on code in PR #20159:
URL: https://github.com/apache/kafka/pull/20159#discussion_r2537961426
##########
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:
##########
@@ -1237,7 +1259,11 @@ private ClusterAndWaitTime waitOnMetadata(String topic,
Integer partition, long
if (metadata.getError(topic) != null &&
metadata.getError(topic).exception() instanceof RetriableException) {
throw new TimeoutException(errorMessage,
metadata.getError(topic).exception());
}
- throw new TimeoutException(errorMessage);
+ if (metadata.getError(topic) != null) {
Review Comment:
@chia7712
Thanks for your comment even if you are in busy!
Before I proceed with removing the Exception and refining the error message,
I would like to discuss a few points regarding your suggestion.
### 1. My perspective on the Exception hierarchy
I have carefully read your comments, but I hold a slightly different
perspective.
`TimeoutException` is already a subclass of `RetryableException`. Therefore,
any logic catching a TimeoutException will inherently treat it as a
`RetryableException`. Furthermore, a `TimeoutException` does not always contain
a cause exception. If the cause is missing (`null`), it implicitly suggests a
context similar to a `NotRetryableException` regarding the root cause, as no
`RetryableException` is explicitly present.
Consequently, I believe that wrapping a `KafkaException` within a
`TimeoutException` is semantically equivalent to having no cause. Additionally,
by including the `KafkaException`, we can effectively convey the expected
message distinct from the existing error message.
### 2. Context regarding KAFKA-17019
As you know, KAFKA-17019 was created because KAFKA-16965 did not capture all
root causes. I think we can build upon our previous discussions to better
understand the current context.
Semantically, when the cause of a `TimeoutException` is `null`, it is
reasonable to view that absence as a `NotRetryableException`. With this in
mind, could we adjust the context of our previous discussion and consider
allowing `TimeoutException` to contain both `RetryableException` and
`NotRetryableException`?
I also believe this approach will not significantly impact the code's
behavior. Since `TimeoutException` is a `RetryableException`, it will generally
trigger a retry. Moreover, given that the cause can be `null`, whether the
underlying cause is a `RetryableException` or not should not affect the retry
logic itself.
### Conclusion
Based on this reasoning, I believe that wrapping `KafkaException` within
`TimeoutException`—and including the expected root cause message there—remains
a valid and effective approach.
I would appreciate your opinion on this.
However, if you think that my opinion is not reasonable to you, please let
me know.
In that case, I will proceed with creating a new commit focused solely on
enhancing the error message and request a review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
