KAFKA-5506: Fix NPE in OffsetFetchRequest.toString #3420
Conversation
|
Review by @dguy. |
| @@ -371,8 +371,7 @@ private void doSend(ClientRequest clientRequest, boolean isInternalRequest, long | |||
| } catch (UnsupportedVersionException e) { | |||
| // If the version is not supported, skip sending the request over the wire. | |||
| // Instead, simply add it to the local queue of aborted requests. | |||
| log.debug("Version mismatch when attempting to send {} to {}", | |||
| clientRequest.toString(), clientRequest.destination(), e); | |||
| log.debug("Version mismatch when attempting to send {} to {}", clientRequest, clientRequest.destination(), e); | |||
There was a problem hiding this comment.
It's a bit odd that we output the full clientRequest and one of its fields separately. We could potentially remove the latter or replace the former with a subset of fields. Either way, we should not be calling toString.
There was a problem hiding this comment.
It does seem a bit odd as ClientRequest#toString has the destination. Should we just remove clientRequest.destination() as it isn't really adding any additional value?
There was a problem hiding this comment.
I think that's fair. I was a bit undecided between doing that and changing the first parameter to be more like the other logging statements in the class where we don't log the full client request. Do you have a preference?
There was a problem hiding this comment.
I'd go for being consistent with the other logging statements
There was a problem hiding this comment.
OK, I replaced clientRequest with the request builder. That would have the information missing from UnsupportedVersionException (the latter would have the api key and version that was requested).
| @@ -371,8 +371,7 @@ private void doSend(ClientRequest clientRequest, boolean isInternalRequest, long | |||
| } catch (UnsupportedVersionException e) { | |||
| // If the version is not supported, skip sending the request over the wire. | |||
| // Instead, simply add it to the local queue of aborted requests. | |||
| log.debug("Version mismatch when attempting to send {} to {}", | |||
| clientRequest.toString(), clientRequest.destination(), e); | |||
| log.debug("Version mismatch when attempting to send {} to {}", clientRequest, clientRequest.destination(), e); | |||
There was a problem hiding this comment.
It does seem a bit odd as ClientRequest#toString has the destination. Should we just remove clientRequest.destination() as it isn't really adding any additional value?
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@hachikuji, maybe you can take a look in this small PR. The important change is that we avoid throwing the NPE when downgrading the request. |
| @@ -371,8 +371,7 @@ private void doSend(ClientRequest clientRequest, boolean isInternalRequest, long | |||
| } catch (UnsupportedVersionException e) { | |||
| // If the version is not supported, skip sending the request over the wire. | |||
| // Instead, simply add it to the local queue of aborted requests. | |||
| log.debug("Version mismatch when attempting to send {} to {}", | |||
| clientRequest.toString(), clientRequest.destination(), e); | |||
| log.debug("Version mismatch when attempting to send {} to {}", builder, clientRequest.destination(), e); | |||
There was a problem hiding this comment.
One thing from the ClientRequest that we don't get from the builder is the correlationId. This is occasionally useful when debugging. If you think it's useful, we might consider adding it to the log lines in doSend as well.
There was a problem hiding this comment.
Hehe, I was debating that. Since other places didn't seem to be doing it, I did not do it here either. I pushed additional commits attempting to do this and improve how we output node information. Please take a look.
This way we don't pay the cost unnecessarily
ijuma
force-pushed
the
kafka-5506-offset-fetch-request-to-string-npe
branch
from
47bdaf8
to
1e353cb
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
…ements NetworkClient's logging improvements: - Include correlation id in a number of log statements - Avoid eager toString call in parameter passed to log.debug - Use node.toString instead of passing a subset of fields to the logger - Use requestBuilder instead of clientRequest in one of the log statements Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Damian Guy <damian.guy@gmail.com>, Jason Gustafson <jason@confluent.io> Closes #3420 from ijuma/kafka-5506-offset-fetch-request-to-string-npe (cherry picked from commit a5c47db) Signed-off-by: Ismael Juma <ismael@juma.me.uk>
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Also include a number of improvements in NetworkClient's logging:
logger
statements