KAFKA-8933; Fix NPE in DefaultMetadataUpdater after authentication failure#7682
Conversation
23a6ef7 to
06442f9
Compare
stanislavkozlovski
left a comment
There was a problem hiding this comment.
Thanks for the PR! I gave it a look mostly to help familiarize myself with the code
There was a problem hiding this comment.
nit: Should we call this maybeAuthException like in the interface?
There was a problem hiding this comment.
Maybe a debug log level is appropriate here. We discussed this way back in #3832 (comment)
I imagine this would look something like
ERROR [Controller id=0, targetBrokerId=0] Connection to node 0 failed authentication due to: Authentication failed due to invalid credentials with SASL mechanism PLAIN (org.apache.kafka.clients.NetworkClient)
ERROR Received authentication failure from brokerId=0 (org.apache.kafka.clients.ManualMetadataUpdater)
There was a problem hiding this comment.
I think debug logging would be sufficient for this one as well as the log entry below for normal disconnect.
There was a problem hiding this comment.
In that case, do these log messages have any value? Perhaps we can get rid of them.
There was a problem hiding this comment.
@rajinisivaram do you know of any cases where the maybeFatalException.get() may not be descriptive enough?
There was a problem hiding this comment.
Shouldn't we call handleDisconnect here?
There was a problem hiding this comment.
My intention was for handleDisconnect to only handle unexpected disconnects. I'll change the name to handleServerDisconnect to make that clearer. The thought, in the context of a MetadataUpdater, was that a server disconnect might trigger an attempt to refresh metadata. Looks like it's just a little more work to push this through and then we can get rid of the requestUpdate API.
There was a problem hiding this comment.
nit: isUpdateDue() already checks for !hasFetchInProgress()
There was a problem hiding this comment.
bootstrap() itself should mandate an update, right? Do we need to call requestUpdate?
There was a problem hiding this comment.
Do we have tests for this?
There was a problem hiding this comment.
I don't think this was strictly necessary. I added it during one of the refactors. I will remove it.
|
@rajinisivaram maybe you can help review this. |
rajinisivaram
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for the PR. Left a minor comment and @stanislavkozlovski had some good comments too, apart from those LGTM.
There was a problem hiding this comment.
I think debug logging would be sufficient for this one as well as the log entry below for normal disconnect.
06442f9 to
7f79b99
Compare
|
@stanislavkozlovski @rajinisivaram This is ready for another look when you get a chance. |
stanislavkozlovski
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the PR :)
rajinisivaram
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for the updates, LGTM
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
…ilure (#7682) This patch fixes an NPE in `DefaultMetadataUpdater` due to an inconsistency in event expectations. Whenever there is an authentication failure, we were treating it as a failed update even if was from a separate connection from an inflight metadata request. This patch fixes the problem by making the `MetadataUpdater` api clearer in terms of the events that are handled. Reviewers: Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Rajini Sivaram <rajinisivaram@googlemail.com>
…ilure (apache#7682) This patch fixes an NPE in `DefaultMetadataUpdater` due to an inconsistency in event expectations. Whenever there is an authentication failure, we were treating it as a failed update even if was from a separate connection from an inflight metadata request. This patch fixes the problem by making the `MetadataUpdater` api clearer in terms of the events that are handled. Reviewers: Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Rajini Sivaram <rajinisivaram@googlemail.com>
This patch fixes an NPE in
DefaultMetadataUpdaterdue to an inconsistency in event expectations. Whenever there is an authentication failure, we were treating it as a failed update even if was from a separate connection from an inflight metadata request. This patch fixes the problem by making theMetadataUpdaterapi clearer in terms of the events that are handled.Committer Checklist (excluded from commit message)