Skip to content

KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest#16136

Merged
mimaison merged 4 commits intoapache:trunkfrom
tinaselenge:KAFKA-16865
Jun 12, 2024
Merged

KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest#16136
mimaison merged 4 commits intoapache:trunkfrom
tinaselenge:KAFKA-16865

Conversation

@tinaselenge
Copy link
Copy Markdown
Contributor

@tinaselenge tinaselenge commented May 30, 2024 •
edited
Loading

KAFKA-16865: Admin.describeTopics behavior change after KIP-966

Committer Checklist (excluded from commit message)

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

@tinaselenge tinaselenge changed the title Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest May 30, 2024
AndrewJSchofield
AndrewJSchofield requested changes May 30, 2024
View reviewed changes
Comment thread clients/src/main/resources/common/message/DescribeTopicPartitionsRequest.json Outdated
mimaison
mimaison reviewed May 31, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison 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 the PR! The change looks good but can we add a test?

@CalvinConfluent
Copy link
Copy Markdown
Contributor

CalvinConfluent commented Jun 7, 2024

Thanks for the change! we would better have a UT to test the describeTopics for admin client. There is no UT coverage for the includeAuthorizedOptions.

apoorvmittal10
apoorvmittal10 approved these changes Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

LGTM! I agree with @mimaison that we need unit tests for the PR.

@AndrewJSchofield
Copy link
Copy Markdown
Member

AndrewJSchofield commented Jun 10, 2024

@tinaselenge Are you going to be able to add a unit test? It would be good to get this PR into 3.8.

@tinaselenge
Copy link
Copy Markdown
Contributor Author

tinaselenge commented Jun 11, 2024

Sorry, yes, I will add it in today.

@tinaselenge
Copy link
Copy Markdown
Contributor Author

tinaselenge commented Jun 11, 2024

Thanks everyone for reviewing the PR. I have now added an unit test for the change.

AndrewJSchofield
AndrewJSchofield approved these changes Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield 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 adding the test. Looks good to me.

mimaison
mimaison reviewed Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks @tinaselenge for the update. It looks good overall, I left a couple of minor suggestions.

Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated
CalvinConfluent
CalvinConfluent approved these changes Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@CalvinConfluent CalvinConfluent left a comment

Choose a reason for hiding this comment

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

LGTM

chia7712
chia7712 approved these changes Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tinaselenge thanks for nice patch. I have two questions. Please take a look!

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java
mimaison
mimaison approved these changes Jun 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison 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!

@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jun 12, 2024

I'll merge to trunk and 3.8 once the CI completes (assuming no related failures)

@mimaison mimaison merged commit 9368ef8 into apache:trunk Jun 12, 2024
mimaison pushed a commit that referenced this pull request Jun 12, 2024
@tinaselenge @mimaison
KAFKA-16865: Add IncludeTopicAuthorizedOperations option for Describe…
72e72e3 
…TopicPartitionsRequest (#16136)


Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>, Calvin Liu <caliu@confluent.io>, Andrew Schofield <andrew_schofield@live.com>, Apoorv Mittal <amittal@confluent.io>
@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jun 12, 2024

Done

@chia7712 chia7712 mentioned this pull request Jun 12, 2024
Merged
3 tasks
@tinaselenge
Copy link
Copy Markdown
Contributor Author

tinaselenge commented Jun 12, 2024 •
edited
Loading

Thank you @mimaison!

apourchet added a commit to apourchet/kafka that referenced this pull request Jun 12, 2024
@apourchet
Squashed commit of the following:
4ba836d 
commit 9368ef8
Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com>
Date:   Wed Jun 12 16:04:24 2024 +0100

    KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest (apache#16136)

    Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>, Calvin Liu <caliu@confluent.io>, Andrew Schofield <andrew_schofield@live.com>, Apoorv Mittal <amittal@confluent.io>

commit 46eb081
Author: gongxuanzhang <gongxuanzhang@foxmail.com>
Date:   Wed Jun 12 22:23:39 2024 +0800

    KAFKA-10787 Apply spotless to log4j-appender, trogdor, jmh-benchmarks, examples, shell and generator (apache#16296)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 79b9c44
Author: gongxuanzhang <gongxuanzhang@foxmail.com>
Date:   Wed Jun 12 22:19:47 2024 +0800

    KAFKA-10787 Apply spotless to connect module (apache#16299)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit b5fb654
Author: Abhijeet Kumar <abhijeet.cse.kgp@gmail.com>
Date:   Wed Jun 12 19:47:46 2024 +0530

    KAFKA-15265: Dynamic broker configs for remote fetch/copy quotas (apache#16078)

    Reviewers: Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Satish Duggana <satishd@apache.org>

commit faee6a4
Author: Dmitry Werner <grimekillah@gmail.com>
Date:   Wed Jun 12 15:44:11 2024 +0500

    MINOR: Use predetermined dir IDs in ReplicationQuotasTest

    Use predetermined directory IDs instead of Uuid.randomUuid() in ReplicationQuotasTest.

    Reviewers: Igor Soarez <soarez@apple.com>

commit 638844f
Author: David Jacot <djacot@confluent.io>
Date:   Wed Jun 12 08:29:50 2024 +0200

    KAFKA-16770; [2/2] Coalesce records into bigger batches (apache#16215)

    This patch is the continuation of apache#15964. It introduces the records coalescing to the CoordinatorRuntime. It also introduces a new configuration `group.coordinator.append.linger.ms` which allows administrators to chose the linger time or disable it with zero. The new configuration defaults to 10ms.

    Reviewers: Jeff Kim <jeff.kim@confluent.io>, Justine Olshan <jolshan@confluent.io>

commit 39ffdea
Author: Bruno Cadonna <cadonna@apache.org>
Date:   Wed Jun 12 07:51:38 2024 +0200

    KAFKA-10199: Enable state updater by default (apache#16107)

    We have already enabled the state updater by default once.
    However, we ran into issues that forced us to disable it again.
    We think that we fixed those issues. So we want to enable the
    state updater again by default.

    Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Matthias J. Sax <matthias@confluent.io>

commit 0782232
Author: Antoine Pourchet <antoine@responsive.dev>
Date:   Tue Jun 11 22:31:43 2024 -0600

    KAFKA-15045: (KIP-924 pt. 22) Add RackAwareOptimizationParams and other minor TaskAssignmentUtils changes (apache#16294)

    We now provide a way to more easily customize the rack aware
    optimizations that we provide by way of a configuration class called
    RackAwareOptimizationParams.

    We also simplified the APIs for the optimizeXYZ utility functions since
    they were mutating the inputs anyway.

    Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>

commit 226ac5e
Author: Murali Basani <muralidhar.basani@aiven.io>
Date:   Wed Jun 12 05:38:50 2024 +0200

    KAFKA-16922 Adding unit tests for NewTopic  (apache#16255)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 23fe71d
Author: Abhijeet Kumar <abhijeet.cse.kgp@gmail.com>
Date:   Wed Jun 12 06:27:02 2024 +0530

    KAFKA-15265: Integrate RLMQuotaManager for throttling copies to remote storage (apache#15820)

    - Added the integration of the quota manager to throttle copy requests to the remote storage. Reference KIP-956
    - Added unit-tests for the copy throttling logic.

    Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>

commit 2fa2c72
Author: Chris Egerton <chrise@aiven.io>
Date:   Tue Jun 11 23:15:07 2024 +0200

    MINOR: Wait for embedded clusters to start before using them in Connect OffsetsApiIntegrationTest (apache#16286)

    Reviewers: Greg Harris <greg.harris@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

@mimaison mimaison mimaison approved these changes

@apoorvmittal10 apoorvmittal10 apoorvmittal10 approved these changes

@AndrewJSchofield AndrewJSchofield AndrewJSchofield approved these changes

+1 more reviewer

@CalvinConfluent CalvinConfluent CalvinConfluent approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tinaselenge @CalvinConfluent @AndrewJSchofield @mimaison @apoorvmittal10 @chia7712