-
Notifications
You must be signed in to change notification settings - Fork 14.7k
MINOR: Suppress warning logs for UnsupportedVersionException in ClientTelemetryReporter #20722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR to remove an annoying log message that's appeared recently :)
| again. We may disconnect from the broker and connect to a broker that supports client | ||
| telemetry. | ||
| */ | ||
| boolean shouldWait = isRetryable(maybeFatalException); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd prefer to see
if (isRetryable(maybeFatalException))
There's no need for a shouldWait variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also, given that it's a RetriableException, why not isRetriable too?
| updateErrorResult(DEFAULT_PUSH_INTERVAL_MS, nowMs); | ||
| } else { | ||
| log.warn("Received unrecoverable error from broker, disabling telemetry"); | ||
| boolean shouldLog = shouldLog(maybeFatalException); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DL1231 thanks for this fix.
@AndrewJSchofield Since KAFKA-19747 was merged into trunk, 4.1, and 4.0, this fix should be backported to those branches as well. WDYT?
…tTelemetryReporter (#20722) see https://github.com/apache/kafka/pull/20661/files#r2433576874 Suppress warning logs for `UnsupportedVersionException` in `ClientTelemetryReporter`. Reviewers: Andrew Schofield <aschofield@confluent.io>
…tTelemetryReporter (#20722) see https://github.com/apache/kafka/pull/20661/files#r2433576874 Suppress warning logs for `UnsupportedVersionException` in `ClientTelemetryReporter`. Reviewers: Andrew Schofield <aschofield@confluent.io>
see https://github.com/apache/kafka/pull/20661/files#r2433576874
Suppress warning logs for
UnsupportedVersionExceptioninClientTelemetryReporter.Reviewers: Andrew Schofield aschofield@confluent.io