-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
KAFKA-9938; Debug consumer should be able to fetch from followers #8585
Conversation
// Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata) | ||
val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty) | ||
// Restrict fetching to leader if request is from follower or from an ordinary consumer | ||
// with an older version (which is implied by no ClientMetadata) |
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.
Seems like this comment is a bit redundant. The main useful info is that no ClientMetadata means older version, could we have a method name that made that clear and then the comment would not be needed?
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.
The second question is: do we want to allow this debugging consumer id behavior at all? Is it useful in the new world?
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.
Yeah, I was debating the latter. It's probably unlikely that this is used outside of the ReplicaVerificationTest. As far as I know, no one has filed an issue about it, so maybe we could drop this support. I decided to err on the side of compatibility, but I could probably be convinced to change my mind.
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.
Can we remove DebuggingConsumerId
altogether or is there any functionality that it provides in today's world? If it doesn't provide any useful functionality now, I would just remove it and it's one less concept to support and think about. As you said, I don't think anyone actually uses this outside of the ReplicaVerificationTool
.
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.
If there are any use cases which rely on it, users can upgrade their clients to regain the ability. Since no one has evidently noticed that this functionality was lost, I think it's probably fine to remove it.
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.
Sounds good.
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
retest this please |
Only known flaky test failures:
|
I am going to close this. Since the new |
There was a minor regression in the fetch protocol introduced in 2.4 as part of the KIP-392 work. We should allow the "debug consumer" to read from follower regardless of the protocol version.
Committer Checklist (excluded from commit message)