chickenchickenlove commented on code in PR #20159:
URL: https://github.com/apache/kafka/pull/20159#discussion_r2206791453
##########
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:
##########
@@ -1225,7 +1230,9 @@ private ClusterAndWaitTime waitOnMetadata(String topic,
Integer partition, long
if (metadata.getError(topic) != null) {
throw new TimeoutException(errorMessage,
metadata.getError(topic).exception());
}
- throw new TimeoutException(errorMessage);
+ if (ex.getCause() != null)
+ throw new TimeoutException(errorMessage, ex.getCause());
+ throw new TimeoutException(errorMessage, new
PotentialCauseException("Metadata update timed out ― topic missing, auth
denied, broker/partition unavailable, or client sender/buffer stalled."));
Review Comment:
@chia7712
Thank you for considering my proposal and for sharing your valuable thoughts.
As you mentioned, I also think it’s a good idea to reuse an existing
Exception class.
Personally, I think using `KafkaException` would be a better approach.
Since there can be multiple potential causes for a `TimeoutException` in
certain code paths, it might be difficult to pinpoint the exact cause. In such
cases, it could be unclear which subclass of `TimeoutException` should be used.
(https://github.com/apache/kafka/commit/e032a360708cec2284f714e4cae388066064d61c)
So, my suggestion is as follows:
- Include a `KafkaException` as the root cause of the `TimeoutException`,
and describe the possible scenario in the error message of the `KafkaException`.
- Let the detailed error information be available via the root cause of the
`TimeoutException`.
- Keep the current message format of the `TimeoutException` itself (which
currently only includes the elapsed time before it expired).
I’m thinking of revising the PR in this direction.
This way, I believe we can preserve the current semantics of
`TimeoutException` while still conveying helpful contextual information (such
as a potential cause) when necessary.
In the future, if there's a need to branch logic based on a more specific
cause of the timeout—even if no actual exception was thrown—then the developer
could define a concrete `PotentialCauseException` class as needed.
Also, if this direction sounds reasonable, I think it wouldn’t require a KIP
change.
What do you think?
Please share your opinion 🙇♂️
--
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]
