Skip to content

KAFKA-12369; Implement ListTransactions API#10206

Merged
hachikuji merged 10 commits intoapache:trunkfrom
hachikuji:KAFKA-12369
Mar 2, 2021
Merged

KAFKA-12369; Implement ListTransactions API#10206
hachikuji merged 10 commits intoapache:trunkfrom
hachikuji:KAFKA-12369

Conversation

@hachikuji
Copy link
Copy Markdown
Contributor

This patch implements the ListTransactions API as documented in KIP-664: https://cwiki.apache.org/confluence/display/KAFKA/KIP-664%3A+Provide+tooling+to+detect+and+abort+hanging+transactions. This is only the server-side implementation and does not contain the Admin API.

Committer Checklist (excluded from commit message)

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

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.

LGTM overall, some questions are left. Please take a look.

Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
"about": "The transaction states to filter by: if empty, all transactions are returned; if non-empty, then only transactions matching one of the filtered states will be returned"
},
{ "name": "ProducerIdFilter", "type": "[]int64", "versions": "0+",
"about": "The producerIds to filter by: if empty, no transactions will be returned; if non-empty, only transactions which match one of the filtered producerIds will be returned"
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.

no transactions will be returned
^^^^^^^^^^^^^^^^^^^^^^^^^^ this document is a bit weird since the code shows that all transactions are returned if this filter is empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my bad. On a side note, do you think it would be clearer if we used null to indicate the absence of the filter?

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.

On a side note, do you think it would be clearer if we used null to indicate the absence of the filter?

it should be fine in this case. However, the empty collection gets weird as it absolutely gets nothing :(

@hachikuji
Copy link
Copy Markdown
Contributor Author

@chia7712 Thanks, I really appreciate your help reviewing these patches. I pushed a couple comments and left a few responses to your questions.

Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated
Comment thread clients/src/main/resources/common/message/ListTransactionsResponse.json Outdated
Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
@hachikuji
Copy link
Copy Markdown
Contributor Author

@chia7712 I implemented your suggestion to include the unknown states in the response. Let me know how this looks to you.

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.

@hachikuji Thanks for updating code. some comments are left. please take a look.

Comment thread clients/src/main/resources/common/message/ListTransactionsResponse.json Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated
Comment thread clients/src/main/resources/common/message/ListTransactionsResponse.json Outdated
Comment thread core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated
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.

+1 to this nice patch. Two trivial comments are left.

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.

+1 if following question is invalid

Comment thread core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala Outdated
@hachikuji hachikuji merged commit 3708a7c into apache:trunk Mar 2, 2021
ijuma added a commit to ijuma/kafka that referenced this pull request Mar 3, 2021
* apache-github/trunk:
  KAFKA-12400: Upgrade jetty to fix CVE-2020-27223
  MINOR: Fix null exception in coordinator log (apache#10250)
  y
  KAFKA-12375: don't reuse thread.id until a thread has fully shut down (apache#10215)
  KAFKA-12177: apply log start offset retention before time and size based retention (apache#10216)
  KAFKA-12369; Implement `ListTransactions` API (apache#10206)
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.

3 participants