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.

Sign up for GitHub

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

Jump to bottom

KAFKA-14509: [4/4] Handle includeAuthorizedOperations #16158

Merged
merged 7 commits into from
Jun 10, 2024
Merged

KAFKA-14509: [4/4] Handle includeAuthorizedOperations #16158

merged 7 commits into from
Jun 10, 2024
+50 −10

Conversation

riedelmax
Copy link
Contributor

Last PR for KAFKA-14509

@dajac dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label May 31, 2024
@dajac
Copy link
Member

dajac commented May 31, 2024

@riedelmax Thanks for the patch. Could you please extend unit and integration tests to cover this change?

@chia7712
Copy link
Member

chia7712 commented Jun 1, 2024

out of curiosity, do we have IT for that option? I grep code base and it seems the related ITs are running with old coordinator/protocol. For example:

  1. kafka/core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala

    Line 1213 in fb566e4

    val describeWithFakeGroupResult = client.describeConsumerGroups(Seq(testGroupId, fakeGroupId).asJava,

  2. kafka/core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala

    Line 139 in fb566e4

    val describeConsumerGroupsResult = client.describeConsumerGroups(Seq(Group1, Group2, Group3).asJava,

dajac
dajac reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@riedelmax Thanks for the update. I left one more comment. I also wonder if we could add a test to ConsumerGroupDescribeRequestsTest to cover the change. Have you considered it?

core/src/test/scala/unit/kafka/server/KafkaApisTest.scala Outdated Show resolved Hide resolved
@riedelmax
Copy link
Contributor Author

@dajac thanks for checking so quickly. I will have time on friday to do the integration tet

@riedelmax
Copy link
Contributor Author

@chia7712 im not sure if I understand you correctlu. We have some integration tests in ConsumerGroupDescribeRequestTest

@chia7712
Copy link
Member

chia7712 commented Jun 4, 2024

We have some integration tests in ConsumerGroupDescribeRequestTest

thanks for this reminder. I neglect that before :(

@dajac
Copy link
Member

dajac commented Jun 7, 2024

@riedelmax There are a bunch of conflicts. Could you please fix them? Regarding ConsumerGroupDescribeRequestsTest, is it possible to extend it too?

riedelmax added 2 commits June 7, 2024 16:52
@riedelmax
KAFKA-14509: extend integration test with includeAuthorizedOperations…
1d9bf29 
… flag
@riedelmax
Merge remote-tracking branch 'upstream/trunk' into kafka-14509-handle…
e64ccc1 
…-authorizedoperations

# Conflicts:
#	core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
@chia7712
Copy link
Member

chia7712 commented Jun 8, 2024

@riedelmax please rebase code to have the fix #16249

@riedelmax
Copy link
Contributor Author

@chia7712 would you mind to explain how #16249 effects this PR? if there are no conflicts with trunk i dont need to rebase or merge manually right?

@chia7712
Copy link
Member

chia7712 commented Jun 9, 2024

@riedelmax #16249 fix the blocked tests. Without that fix, the CI will get timeout when running your PR

@chia7712
Copy link
Member

chia7712 commented Jun 9, 2024

I prefer to check all failed tests before merging. And the last commit of this PR does not have completed CI. That is why I suggest to rebase code to trigger QA again.

@dajac
Copy link
Member

dajac commented Jun 10, 2024

Merged trunk to include #16249.

dajac
dajac approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. We need to cherry-pick it to 3.8 too.

@dajac
Copy link
Member

dajac commented Jun 10, 2024

CI does not seem to be working...

@dajac dajac merged commit 40de07d into apache:trunk Jun 10, 2024
1 check failed
dajac pushed a commit that referenced this pull request Jun 10, 2024
@riedelmax @dajac
KAFKA-14509; [4/4] Handle includeAuthorizedOperations in ConsumerGrou…
db3bf4a 
…pDescribe API (#16158)

This patch implements the handling of `includeAuthorizedOperations` flag in the ConsumerGroupDescribe API.

Reviewers: David Jacot <[email protected]>
@dajac
Copy link
Member

dajac commented Jun 10, 2024

Merged to trunk and to 3.8.

gongxuanzhang pushed a commit to gongxuanzhang/kafka that referenced this pull request Jun 12, 2024
@riedelmax @gongxuanzhang
KAFKA-14509; [4/4] Handle includeAuthorizedOperations in ConsumerGrou…
34d1bf4 
…pDescribe API (apache#16158)

This patch implements the handling of `includeAuthorizedOperations` flag in the ConsumerGroupDescribe API.

Reviewers: David Jacot <[email protected]>
@riedelmax riedelmax deleted the kafka-14509-handle-authorizedoperations branch August 9, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dajac dajac dajac approved these changes

Assignees
No one assigned
Labels
KIP-848 The Next Generation of the Consumer Rebalance Protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@riedelmax @dajac @chia7712