-
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-16265: KIP-994 (Part 1) Minor Enhancements to ListTransactionsRequest #15384
Conversation
clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/TransactionsCommand.java
Outdated
Show resolved
Hide resolved
clients/src/main/resources/common/message/ListTransactionsRequest.json
Outdated
Show resolved
Hide resolved
clients/src/main/resources/common/message/ListTransactionsResponse.json
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java
Outdated
Show resolved
Hide resolved
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.
Thanks @yyu1993 I will wait for the tests to run and see if @RamanVerma has any comments.
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.
thanks @yyu1993
I have left a few comments
clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/scala/kafka/coordinator/transaction/TransactionCoordinator.scala
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/TransactionsCommand.java
Outdated
Show resolved
Hide resolved
core/src/test/scala/unit/kafka/coordinator/transaction/TransactionStateManagerTest.scala
Show resolved
Hide resolved
core/src/test/scala/unit/kafka/coordinator/transaction/TransactionStateManagerTest.scala
Show resolved
Hide resolved
I think the unit tests look good. Is it possible to write/edit (in case we have one existing) an integration test that builds a request and validates the response from broker. You will need to feed in some transactions to the broker transactionManager cache as your test data. |
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.
thanks Yang Yu, LGTM
Something wrong with the build so I will restart. |
…equest (apache#15384) Introduces a new filter in ListTransactionsRequest API. This enables caller to filter on transactions that have been running for longer than a certain duration of time. This PR includes the following changes: bumps version for ListTransactionsRequest API to 1. Set the durationFilter to -1L when communicating with an older broker that does not support version 1. bumps version for ListTransactionsResponse to 1 without changing the response structure. adds durationFilter option to kafka-transactions.sh --list Tests: Client side test to build request with correct combination of duration filter and API version: testBuildRequestWithDurationFilter Server side test to filter transactions based on duration: testListTransactionsFiltering Added test case for kafka-transactions.sh change in TransactionsCommandTest Reviewers: Justine Olshan <jolshan@confluent.io>, Raman Verma <rverma@confluent.io>
…equest (apache#15384) Introduces a new filter in ListTransactionsRequest API. This enables caller to filter on transactions that have been running for longer than a certain duration of time. This PR includes the following changes: bumps version for ListTransactionsRequest API to 1. Set the durationFilter to -1L when communicating with an older broker that does not support version 1. bumps version for ListTransactionsResponse to 1 without changing the response structure. adds durationFilter option to kafka-transactions.sh --list Tests: Client side test to build request with correct combination of duration filter and API version: testBuildRequestWithDurationFilter Server side test to filter transactions based on duration: testListTransactionsFiltering Added test case for kafka-transactions.sh change in TransactionsCommandTest Reviewers: Justine Olshan <jolshan@confluent.io>, Raman Verma <rverma@confluent.io>
…equest (apache#15384) Introduces a new filter in ListTransactionsRequest API. This enables caller to filter on transactions that have been running for longer than a certain duration of time. This PR includes the following changes: bumps version for ListTransactionsRequest API to 1. Set the durationFilter to -1L when communicating with an older broker that does not support version 1. bumps version for ListTransactionsResponse to 1 without changing the response structure. adds durationFilter option to kafka-transactions.sh --list Tests: Client side test to build request with correct combination of duration filter and API version: testBuildRequestWithDurationFilter Server side test to filter transactions based on duration: testListTransactionsFiltering Added test case for kafka-transactions.sh change in TransactionsCommandTest Reviewers: Justine Olshan <jolshan@confluent.io>, Raman Verma <rverma@confluent.io>
Introduces a new filter in ListTransactionsRequest API. This enables caller to filter on transactions that have been running for longer than a certain duration of time.
This PR includes the following changes:
kafka-transactions.sh --list
Tests:
testBuildRequestWithDurationFilter
testListTransactionsFiltering
kafka-transactions.sh
change inTransactionsCommandTest
Committer Checklist (excluded from commit message)