Skip to content

KAFKA-12709; Add Admin API for ListTransactions#10616

Merged
hachikuji merged 3 commits into
apache:trunkfrom
hachikuji:KAFKA-12709
Jun 2, 2021
Merged

KAFKA-12709; Add Admin API for ListTransactions#10616
hachikuji merged 3 commits into
apache:trunkfrom
hachikuji:KAFKA-12709

Conversation

@hachikuji

Copy link
Copy Markdown
Contributor

This patch adds Admin support for the listTransactions API, which was added by KIP-664. Similar to listConsumerGroups, the new listTransactions API is intended to be sent to all brokers. This did not fit very well with the new AdminApiDriver that was used for the other KIP-664 APIs, so I had to refactor a bit. The way I ultimately handled it is to treat the brokerId as the key that needs to be mapped. Unlike with the other cases, the set of brokerIds is not known ahead of time and has to be discovered through a Metadata lookup (just as with listConsumerGroups). Because the future associated with ListTransactionsResult is more complex, I ended up externalizing future completion in a new AdminApiFuture object which is passed to AdminApiDriver during construction.

One minor change that is also worth calling out is the removal of AdminApiHandler.Keys. This was previously used to indicate the sets of statically and dynamically mapped keys. Instead, I now handle this with a new StaticBrokerStrategy. The static scope is indicated by returning an ApiRequestScope object which has a specific destination broker set.

Committer Checklist (excluded from commit message)

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

@dajac dajac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hachikuji Thanks for the PR. I just made a first pass on it and I left a few minor comments. Overall, the approach seems reasonable to me.

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/Admin.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsResult.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsResult.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsResult.java Outdated

@dajac dajac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hachikuji The PR LGTM, thanks. I have left few minor nits about the error messages.

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsResult.java Outdated
@hachikuji hachikuji merged commit 2d7a4ed into apache:trunk Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants