-
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-14744; NPE while converting OffsetFetch from version < 8 to version >= 8 #13295
Conversation
@@ -126,6 +126,56 @@ class OffsetFetchRequestTest extends BaseRequestTest { | |||
} | |||
} | |||
|
|||
@Test | |||
def testOffsetFetchRequestAllOffsetsSingleGroup(): Unit = { |
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.
This one is based on testOffsetFetchRequestSingleGroup
. I kept the exact same assertions.
buildRequest(new OffsetFetchRequest.Builder( | ||
"group-1", | ||
false, | ||
null, // all offsets. |
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.
This is the important bit. We need to test with null
.
Could it produce NPE too? For example, server receives request from older client but the request fails due to authorization ( https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L1362 ) |
Topics is only nullable since version 2: https://github.com/apache/kafka/blob/trunk/clients/src/main/resources/common/message/OffsetFetchRequest.json#L41. So it should not happen there. |
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
While refactoring the OffsetFetch handling in KafkaApis, we introduced a NullPointerException (NPE). The NPE arises when the FetchOffset API is called with a client using a version older than version 8 and using null for the topics to signal that all topic-partition offsets must be returned. This means that this bug mainly impacts admin tools. The consumer does not use null.
This NPE is here: 24a8642#diff-0f2f19fd03e2fc5aa9618c607b432ea72e5aaa53866f07444269f38cb537f3feR237.
We missed this during the refactor because we had no tests in place to test this mode.
Committer Checklist (excluded from commit message)