KAFKA-19073: add transactional ID pattern filter to ListTransactions #19355

CalvinConfluent · 2025-04-02T22:38:08Z

https://issues.apache.org/jira/browse/KAFKA-19073

Reviewers: Justine Olshan [email protected], Ken Huang
[email protected], Kuan-Po Tseng [email protected], Chia-Ping
Tsai [email protected]

github-actions · 2025-04-10T03:20:20Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

chia7712

@CalvinConfluent thanks for this patch!

core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala

clients/src/main/resources/common/message/ListTransactionsRequest.json

core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala

clients/src/main/java/org/apache/kafka/clients/admin/ListTransactionsOptions.java

m1a2st

Thanks @CalvinConfluent for this patch, a little comments

clients/src/test/java/org/apache/kafka/clients/admin/internals/ListTransactionsHandlerTest.java

tools/src/test/java/org/apache/kafka/tools/TransactionsCommandTest.java

CalvinConfluent · 2025-04-28T15:44:31Z

@chia7712 @m1a2st The KIP-1152 has been accepted! Can you help take another look? Thanks!

chia7712

@CalvinConfluent thanks for this patch. one small comment is left. PTAL

chia7712 · 2025-04-29T14:06:30Z

clients/src/main/java/org/apache/kafka/clients/admin/internals/ListTransactionsHandler.java

@@ -75,6 +75,9 @@ public ListTransactionsRequest.Builder buildBatchedRequest(
            .map(TransactionState::toString)
            .collect(Collectors.toList()));
        request.setDurationFilter(options.filteredDuration());
+        if (!options.filteredTransactionalIdPattern().isEmpty()) {


filteredTransactionalIdPattern is nullable, so should we add null check?

For the filteredTransactionalIdPattern in the ListTransactionsOptions, the default value is "" and we only update it if the user sets the --transactional-id-pattern. It should be valid.

or could you please add null check to ListTransactionsOptions#filterOnTransactionalIdPattern if we don't want to user to set it to null.

Sounds good, updated.

clients/src/main/java/org/apache/kafka/common/requests/ListTransactionsRequest.java

jolshan · 2025-04-29T18:57:39Z

clients/src/main/resources/common/message/ListTransactionsRequest.json

@@ -30,6 +32,9 @@
    },
    { "name": "DurationFilter", "type": "int64", "versions": "1+", "default": -1,
      "about": "Duration (in millis) to filter by: if < 0, all transactions will be returned; otherwise, only transactions running longer than this duration will be returned."
+    },
+    { "name": "TransactionalIdPattern", "type": "string", "versions": "2+", "nullableVersions": "2+", "default": "null",


I'm a little confused by why we convert null to "" on the client side if we allow null and it's the same result as "" (but we do the extra step of filtering for "" on the server side)

Yeah, I think it is because the other regex fields allow null. So we want to get they aligned, like SubscribedTopicRegex in ConsumerGroupHeartbeatRequest. As for converting to an empty string, I find it is convenient to handle in code.

Any reason why we can't just accept null on the client side? We convert to "" now.

I have updated the client side not to convert null in ListTransactionsOptions#filterOnTransactionalIdPattern. Also adding necessary null handlings.

jolshan · 2025-04-29T23:47:45Z

core/src/main/scala/kafka/server/KafkaApis.scala

@@ -2472,7 +2472,17 @@ class KafkaApis(val requestChannel: RequestChannel,
    val filteredProducerIds = listTransactionsRequest.data.producerIdFilters.asScala.map(Long.unbox).toSet
    val filteredStates = listTransactionsRequest.data.stateFilters.asScala.toSet
    val durationFilter = listTransactionsRequest.data.durationFilter()
-    val response = txnCoordinator.handleListTransactions(filteredProducerIds, filteredStates, durationFilter)
+    val transactionalIdPatternFilter = if (listTransactionsRequest.data.transactionalIdPattern == null) {
+      ""


Do we need this as well? We have a null check in TransactionStateManager

val pattern = if (filterTransactionalIdPattern.nonEmpty) { try { Pattern.compile(filterTransactionalIdPattern) } catch { case e: PatternSyntaxException => throw new InvalidRegularExpression(String.format("Transaction ID pattern `%s` is not a valid regular expression: %s.", filterTransactionalIdPattern, e.getMessage)) } } else null

It seems TransactionStateManager does not check the null for filterTransactionalIdPattern

I see. I was looking at the lines directly above where we do check for null. It's a bit confusing which things check for null vs empty 😓

The main reason to convert null to an empty string is to avoid null handling in the downstream. But yeah, looks like there is only one place to check null. Updated to use null in the code path.

jolshan

Thanks Calvin!

brandboat

LGTM, thanks!

CalvinConfluent · 2025-05-01T16:16:22Z

@chia7712 Any other comments? Thanks!

…pache#19355) Propose adding a new filter TransactionalIdPattern. This transaction ID pattern filter works as AND with the other transaction filters. Also, it is empowered with Re2j. KIP: https://cwiki.apache.org/confluence/x/4gm9F Reviewers: Justine Olshan <[email protected]>, Ken Huang <[email protected]>, Kuan-Po Tseng <[email protected]>, Chia-Ping Tsai <[email protected]>

KAFKA-19073: add transactional ID prefix filter to ListTransactions

69a4383

github-actions bot added triage PRs from the community core Kafka Broker tools clients labels Apr 2, 2025

CalvinConfluent changed the title ~~KAFKA-19073: add transactional ID prefix filter to ListTransactions~~ KAFKA-19073: add transactional ID pattern filter to ListTransactions Apr 4, 2025

CalvinConfluent added 3 commits April 4, 2025 10:50

Using regex pattern instead of prefix

dfe0e1c

Minor

98ae820

Make the pattern nullable

ad6da8c

github-actions bot added the needs-attention label Apr 10, 2025

chia7712 added the ci-approved label Apr 15, 2025

chia7712 reviewed Apr 15, 2025

View reviewed changes

m1a2st reviewed Apr 16, 2025

View reviewed changes

github-actions bot removed needs-attention triage PRs from the community labels Apr 16, 2025

Update according to the KIP update

b13a421

github-actions bot added the build Gradle build or GitHub Actions label Apr 23, 2025

CalvinConfluent added 2 commits April 23, 2025 14:41

Minor

b93a188

Minor

620a91b

CalvinConfluent requested review from chia7712 and m1a2st April 24, 2025 00:19

Minor

55c4e30

chia7712 reviewed Apr 29, 2025

View reviewed changes

Address comments

a826891

brandboat reviewed Apr 29, 2025

View reviewed changes

clients/src/main/java/org/apache/kafka/common/requests/ListTransactionsRequest.java Outdated Show resolved Hide resolved

Minor

e2a1c7e

CalvinConfluent requested review from chia7712 and brandboat April 29, 2025 16:45

jolshan reviewed Apr 29, 2025

View reviewed changes

Minor

4616e04

CalvinConfluent requested a review from jolshan April 29, 2025 23:07

jolshan reviewed Apr 29, 2025

View reviewed changes

Move from empty string to null

fe71be4

jolshan approved these changes Apr 30, 2025

View reviewed changes

Minor

f705446

brandboat approved these changes May 1, 2025

View reviewed changes

m1a2st approved these changes May 1, 2025

View reviewed changes

chia7712 approved these changes May 1, 2025

View reviewed changes

chia7712 merged commit 0c1fbf3 into apache:trunk May 1, 2025
21 checks passed

KAFKA-19073: add transactional ID pattern filter to ListTransactions #19355

KAFKA-19073: add transactional ID pattern filter to ListTransactions #19355

Uh oh!

Conversation

CalvinConfluent commented Apr 2, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 10, 2025

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

m1a2st left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CalvinConfluent commented Apr 28, 2025

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jolshan Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolshan left a comment

Choose a reason for hiding this comment

Uh oh!

brandboat left a comment

Choose a reason for hiding this comment

Uh oh!

CalvinConfluent commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

CalvinConfluent commented Apr 2, 2025 •

edited by github-actions bot

Loading

jolshan Apr 29, 2025 •

edited

Loading