AndrewJSchofield commented on code in PR #20722:
URL: https://github.com/apache/kafka/pull/20722#discussion_r2440647354
##########
clients/src/main/java/org/apache/kafka/common/telemetry/internals/ClientTelemetryReporter.java:
##########
@@ -868,10 +875,14 @@ private void handleFailedRequest(boolean shouldWait) {
again. We may disconnect from the broker and connect to a
broker that supports client
telemetry.
*/
+ boolean shouldWait = isRetryable(maybeFatalException);
Review Comment:
And also, given that it's a `RetriableException`, why not `isRetriable` too?
##########
clients/src/main/java/org/apache/kafka/common/telemetry/internals/ClientTelemetryReporter.java:
##########
@@ -868,10 +875,14 @@ private void handleFailedRequest(boolean shouldWait) {
again. We may disconnect from the broker and connect to a
broker that supports client
telemetry.
*/
+ boolean shouldWait = isRetryable(maybeFatalException);
if (shouldWait) {
updateErrorResult(DEFAULT_PUSH_INTERVAL_MS, nowMs);
} else {
- log.warn("Received unrecoverable error from broker,
disabling telemetry");
+ boolean shouldLog = shouldLog(maybeFatalException);
Review Comment:
I would also just check `maybeFatalException instanceof
UnsupportedVersionException` rather than hiding the check in a method. Do we
really have to handle the case of the exception being wrapped by another
exception?
##########
clients/src/main/java/org/apache/kafka/common/telemetry/internals/ClientTelemetryReporter.java:
##########
@@ -868,10 +875,14 @@ private void handleFailedRequest(boolean shouldWait) {
again. We may disconnect from the broker and connect to a
broker that supports client
telemetry.
*/
+ boolean shouldWait = isRetryable(maybeFatalException);
Review Comment:
Personally, I'd prefer to see
```
if (isRetryable(maybeFatalException))
```
There's no need for a shouldWait variable.
--
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]
