bbejeck commented on code in PR #20661:
URL: https://github.com/apache/kafka/pull/20661#discussion_r2415129983
##########
clients/src/main/java/org/apache/kafka/common/telemetry/internals/ClientTelemetryReporter.java:
##########
@@ -524,17 +525,41 @@ public void handleResponse(PushTelemetryResponse
response) {
lock.writeLock().unlock();
}
}
+
+ private boolean isRetryable(final KafkaException maybeFatalException) {
+ if (maybeFatalException == null) {
+ return true;
+ }
+
+ Throwable cause;
+ if (maybeFatalException.getClass() == KafkaException.class) {
+ if (maybeFatalException.getCause() == null) {
+ return false;
Review Comment:
>What about the case, when maybeFatalException.getClass() instanceof
RetriableException ? It could be top level, and not have a "cause" exception
set.
I don't follow exactly. The top line covers the case of `new
KafkaException("blah")`. For the case you mention since it's a `Retriable`
instance with no `cause` it would flow through the top `else` block, drop out
of the while loop and return `true`. This case is covered by the test
`ClientTelemetryReporterTest.testHandleFailedGetTelemetrySubscriptionsRequestWithRetriableException`
>Also, if we have a KafkaException, (which is not instanceof Retriable,
should we treat it as fatal right away?
Yes and that's what the first `if` clause covers and the test is
`testHandleFailedRequestWithGenericKafkaException`
>Is the case of KafkaException with a RetriableException a case, that is
really retriable (does this case even exist?) -- Should the top level exception
not be an instanceof RetriableException to begin with?
Maybe, but I'm inclined to leave the code as is.
--
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]
