Skip to content
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-14499: [1/N] Introduce OffsetCommit API version 9 and add new StaleMemberEpochException error #14046

Merged
merged 9 commits into from Jul 21, 2023

Conversation

dajac
Copy link
Contributor

@dajac dajac commented Jul 19, 2023

This patch does a few things:

  1. It introduces version 9 of the OffsetCommit API. This new version has no schema changes but it can return a StaleMemberEpochException if the new consumer group protocol is used. Note the use of "latestVersionUnstable": true in the request schema. This means that this new version is not available yet unless activated.
  2. It renames the generationId field in the request to GenerationIdOrMemberEpoch. This is backward compatible change.
  3. It introduces the new StaleMemberEpochException error.
  4. It does a minor refactoring in OffsetCommitRequest class.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@dajac dajac added the KIP-848 label Jul 19, 2023
@jolshan
Copy link
Contributor

jolshan commented Jul 19, 2023

Note the use of "latestVersionUnstable": true in the request schema. This means that this new version is not available yet unless activated.

This also leaves us room to add the topic ID to this version if we so choose?

@dajac
Copy link
Contributor Author

dajac commented Jul 19, 2023

Note the use of "latestVersionUnstable": true in the request schema. This means that this new version is not available yet unless activated.

This also leaves us room to add the topic ID to this version if we so choose?

Yeah, that's right.

@jolshan
Copy link
Contributor

jolshan commented Jul 19, 2023

Something strange is going on with AuthorizerIntegrationTest in the build, but that might be unrelated. I will look into that.

Copy link
Contributor

@jolshan jolshan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. Left one small question, but it's mostly my curiosity and nonblocking. Let's confirm the test failures are unrelated.

@dajac
Copy link
Contributor Author

dajac commented Jul 19, 2023

Something strange is going on with AuthorizerIntegrationTest in the build, but that might be unrelated. I will look into that.

@jolshan I found the issue related to AuthorizerIntegrationTest failures. I just pushed a fix. See last commit.

@jolshan
Copy link
Contributor

jolshan commented Jul 19, 2023

Thanks for the fix @dajac let's just confirm the build and we should be good :)

@dajac
Copy link
Contributor Author

dajac commented Jul 20, 2023

@jolshan I was actually thinking about the AuthorizerIntegrationTest failures overnight and I found an issue with the latestVersionUnstable flag. Let me try to explain.

The latestVersionUnstable is used on the broker side to ensure that an unreleased/unstable version is not exposed by the broker. That's fine. However, it does not guarantee that a client having an unreleased/unstable version is not going to use it.

Let's take this change as an example. The version 9 will be shipped in the next release even if we don't want to use it because the schema is there. So the client knows about it and may use it if the broker eventually supports the version. The issue is that the release version my be different from the one shipped so the client would get an error.

I have updated the AbstractRequest.Builder to be defensive and only consider stable versions. If the user wants to construct a request for an unstable version, it has to specify it explicitly. This guarantee that even if an unstable version is shipped, it is never used.

@jolshan
Copy link
Contributor

jolshan commented Jul 20, 2023

@jolshan I was actually thinking about the AuthorizerIntegrationTest failures overnight and I found an issue with the latestVersionUnstable flag. Let me try to explain.

I was curious if the unstable version flag was causing issues since I recall some weirdness in tests when I had an unstable version. Makes sense to require the unstable-ness to be explicit, but I will take a second look.

@jolshan
Copy link
Contributor

jolshan commented Jul 20, 2023

Looking at the tests Build / JDK 20 and Scala 2.13 / kafka.server.FetchRequestTest.testCurrentEpochValidationV12() is a bit strange but it only failed on that version. Everything else seems to be familiar-ish flakes

Copy link
Contributor

@jolshan jolshan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for making the unstable versions explicit in the builder now. Changes lgtm

@dajac
Copy link
Contributor Author

dajac commented Jul 21, 2023

Failed tests are unrelated. Merging to trunk.

@dajac dajac merged commit 69659b7 into apache:trunk Jul 21, 2023
1 check failed
@dajac dajac deleted the KAFKA-14499-1 branch July 21, 2023 18:08
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
…taleMemberEpochException error (apache#14046)

This patch does a few things:
1) It introduces version 9 of the OffsetCommit API. This new version has no schema changes but it can return a StaleMemberEpochException if the new consumer group protocol is used. Note the use of `"latestVersionUnstable": true` in the request schema. This means that this new version is not available yet unless activated.
2) It renames the `generationId` field in the request to `GenerationIdOrMemberEpoch`. This is backward compatible change.
3) It introduces the new StaleMemberEpochException error.
4) It does a minor refactoring in OffsetCommitRequest class.

Reviewers: Jeff Kim <jeff.kim@confluent.io>, David Arthur <mumrah@gmail.com>, Justine Olshan <jolshan@confluent.io>
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
…taleMemberEpochException error (apache#14046)

This patch does a few things:
1) It introduces version 9 of the OffsetCommit API. This new version has no schema changes but it can return a StaleMemberEpochException if the new consumer group protocol is used. Note the use of `"latestVersionUnstable": true` in the request schema. This means that this new version is not available yet unless activated.
2) It renames the `generationId` field in the request to `GenerationIdOrMemberEpoch`. This is backward compatible change.
3) It introduces the new StaleMemberEpochException error.
4) It does a minor refactoring in OffsetCommitRequest class.

Reviewers: Jeff Kim <jeff.kim@confluent.io>, David Arthur <mumrah@gmail.com>, Justine Olshan <jolshan@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants