MINOR: Cache topic resolution in TopicIds set #17285

squah-confluent · 2024-09-26T13:29:59Z

Looking up topics in a TopicsImage is relatively slow. Cache the results
in TopicIds to improve assignor performance. In benchmarks, we see a
noticeable improvement in performance in the heterogeneous case.

Before

Benchmark                                       (assignmentType)  (assignorType)  (isRackAware)  (memberCount)  (partitionsToMemberRatio)  (subscriptionType)  (topicCount)  Mode  Cnt    Score   Error  Units
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL           RANGE          false          10000                         10         HOMOGENEOUS          1000  avgt    5   36.400 ± 3.004  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL           RANGE          false          10000                         10       HETEROGENEOUS          1000  avgt    5  158.340 ± 0.825  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL         UNIFORM          false          10000                         10         HOMOGENEOUS          1000  avgt    5    1.329 ± 0.041  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL         UNIFORM          false          10000                         10       HETEROGENEOUS          1000  avgt    5  382.901 ± 6.203  ms/op

After

Benchmark                                       (assignmentType)  (assignorType)  (isRackAware)  (memberCount)  (partitionsToMemberRatio)  (subscriptionType)  (topicCount)  Mode  Cnt    Score   Error  Units
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL           RANGE          false          10000                         10         HOMOGENEOUS          1000  avgt    5   36.465 ± 1.954  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL           RANGE          false          10000                         10       HETEROGENEOUS          1000  avgt    5  114.043 ± 1.424  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL         UNIFORM          false          10000                         10         HOMOGENEOUS          1000  avgt    5    1.454 ± 0.019  ms/op
ServerSideAssignorBenchmark.doAssignment             INCREMENTAL         UNIFORM          false          10000                         10       HETEROGENEOUS          1000  avgt    5  342.840 ± 2.744  ms/op

Based heavily on #16527.

Committer Checklist (excluded from commit message)

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

Looking up topics in a TopicsImage is relatively slow. Cache the results in TopicIds to improve assignor performance.

dajac

@squah-confluent Thanks for the patch. I left a few nits.

dajac · 2024-09-26T18:03:17Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicIds.java

+     *
+     * Provides an implementation of equals and hashCode based on the underlying TopicsImage.
+     */
+    public abstract static class BaseTopicResolver implements TopicResolver {


nit: Should we keep it private?

Yes, it doesn't need to be public.

dajac · 2024-09-26T18:04:43Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicIds.java

        return result;
    }

    @Override
    public String toString() {
        return "TopicIds(topicNames=" + topicNames +
-            ", image=" + image +
+            ", resolver=" + resolver +


nit: If we toString it here, should we add toString methods to the resolvers too?

I'll add a toString implementation that returns <class name>(image=<image>).

Reviewers: David Arthur <[email protected]>

…ly for new consumer with poll(0) (apache#16982) Reviewers: Lianet Magrans <[email protected]>, TaiJuWu <[email protected]>, Kirk True <[email protected]>, TengYao Chi <[email protected]>

…he#16719) Implement server side changes for epoch bump but keep EndTxn as an unstable API until the client side changes are implemented. EndTxnResponse will return the producer ID and epoch for the transaction. Introduces new tagged fields to the TransactionLogValue to persist the clientTransactionVersion, previousProducerId, and nextProducerId to the log so that the state can be reloaded. See KIP-890 for more details. Small updates to naming of lastProducerId -> PreviousProducerId. Also cleans up the many TransactionMetadata constructors. Reviewers: Artem Livshits <[email protected]>, David Jacot <[email protected]>

…active controller is removed (apache#17146) This change fixes a few issues. KAFKA-17608; KRaft controller crashes when active controller is removed When a control batch is committed, the quorum controller currently increases the last stable offset but fails to create a snapshot for that offset. This causes an issue if the quorum controller renounces and needs to revert to that offset (which has no snapshot present). Since the control batches are no-ops for the quorum controller, it does not need to update its offsets for control records. We skip handle commit logic for control batches. KAFKA-17604; Describe quorum output missing added voters endpoints Describe quorum output will miss endpoints of voters which were added via AddRaftVoter. This is due to a bug in LeaderState's updateVoterAndObserverStates which will pull replica state from observer states map (which does not include endpoints). The fix is to populate endpoints from the lastVoterSet passed into the method. Reviewers: José Armando García Sancio <[email protected]>, Colin P. McCabe <[email protected]>, Chia-Ping Tsai <[email protected]>

…pache#17288) While running large scale performance tests, we noticed that the logging on the ConsumerGroupHeartbeat path took a significant amount of CPU. It is mainly due to the very large data structures that we print out. I made a pass on those logs and I switched some of them to debug. Reviewers: Lianet Magrans <[email protected]>

Reviewers: Chia-Ping Tsai <[email protected]>

…consumer.poll (apache#17165) Reviewers: Lianet Magrans <[email protected]>, TaiJuWu <[email protected]>

…java class (apache#16912) Reviewers: Chia-Ping Tsai <[email protected]>

Reviewers: Chia-Ping Tsai <[email protected]>

…the feature command tool (apache#17093) This patch belongs to the ongoing efforts of implementing KIP-1022. Added feature-dependencies command to look up dependencies for a given feature version supplied by --feature flag. If the feature is not known or the version not yet defined, we throw an error. Examples : bin/kafka-feature feature-dependencies --feature transaction.version=2 transaction.version=2 requires: metadata.version=4 (3.3-IV0) (listing any other version dependencies) bin/kafka-feature feature-dependencies --feature metadata.version=17 metadata.version=17 (3.7-IV2) has no dependencies Reviewers: Justine Olshan <[email protected]>, Artem Livshits <[email protected]>

…pache#17182) This PR simply StreamsMetricsImpl to avoid passing in the unused "metric version" parameter. Reviewers: Matthias J. Sax <[email protected]>

…17259) With EOSv1 removed, we don't need to create a producer per task, and thus can simplify the code by removing KafkaClientSupplier from the deeply nested StreamsProducer, to simplify the code. Reviewers: Bill Bejeck <[email protected]>

This patch allows workflow runs to be controlled by the ci-approved label on Pull Requests. Rather than manually approving each workflow run explicitly, committers can now add the appropriate label and the new "CI Requested" and "PR Labeled" workflows will auto-approve the requested run. Reviewers: Chia-Ping Tsai <[email protected]>

…-common (apache#17289) Reviewers: Chia-Ping Tsai <[email protected]>, David Arthur <[email protected]>

…pache#17295) This is a small patch to change the default value of group.consumer.migration.policy to BIDIRECTIONAL. Reviewers: David Jacot <[email protected]>

…che#17244) Reviewers: Kirk True <[email protected]>, Lianet Magrans <[email protected]>

Reviewers: Colin P. McCabe <[email protected]>, Chia-Ping Tsai <[email protected]>, David Arthur <[email protected]>

… tests from EmbeddedZookeeper to KRaft (apache#17016) Migrate the EmbeddedKafkaCluster from the EmbeddedZookeeper to KRaft Reviewers Bill Bejeck <[email protected]>

Fixes a regression introduced by apache#16669 which inadvertently stopped processing SCRAM arguments from kafka-storage.sh Reviewers: Colin P. McCabe <[email protected]>, Federico Valeri <[email protected]>

Reviewers: Chia-Ping Tsai <[email protected]>

apache#17215) Reviewers: Matthias J. Sax <[email protected]>

This reverts commit 1f9c7ae. This reverts commit 0b97fa6. This reverts commit d9254d5. This reverts commit b905a9f. This reverts commit f438bb4. This reverts commit f0a2d43. This reverts commit 945023d. This reverts commit 2df3ff0. This reverts commit 3b555b8. This reverts commit 2059889. This reverts commit fe5056e. This reverts commit 0589e93. This reverts commit 99e9f81. This reverts commit 813d79c. This reverts commit 5aaf623. This reverts commit 0d8485a. This reverts commit 25b048e. This reverts commit 536dfbb. This reverts commit 655cddd. This reverts commit 48c022c. This reverts commit 03b4cdc. This reverts commit a59e8c5.

…-topic-resolution

…resolution Updates for apache#17285

mumrah

@squah-confluent, Seeing that the cache is not actually used outside of tests and benchmarks, I'm guessing this is still WIP.

Does this come down to performance differences between HashMap and PCollectionsImmutableMap?

If we decide we really need faster topic ID to name lookups, I would consider adding it to TopicsImage. Managing a cache outside of the image will be a bit difficult.

Another thing to consider is the lifetime of the cache. Do we really need the ID + name mappings kept in memory forever?

mumrah · 2024-10-01T17:37:42Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicIds.java

+     * Provides an implementation of equals, hashCode and toString based on the underlying
+     * TopicsImage.
+     */
+    private abstract static class BaseTopicResolver implements TopicResolver {


Why do we need this abstract class? Can we just have the interface and concrete class?

I can remove it.

dajac · 2024-10-02T07:22:41Z

@mumrah

Another thing to consider is the lifetime of the cache. Do we really need the ID + name mappings kept in memory forever?

The lifetime of the cache is bound to the call. It is not kept forever.

Does this come down to performance differences between HashMap and PCollectionsImmutableMap?

Yes.

If we decide we really need faster topic ID to name lookups, I would consider adding it to TopicsImage. Managing a cache outside of the image will be a bit difficult.

We could consider this separately. At the moment, we don't really have the time to do it. The current strategy seems to be a good tradeoff at the moment given that it is only bound to the call and not kept forever.

mumrah · 2024-10-02T12:10:14Z

@dajac thanks for the explanation, makes sense. Can we include a javadoc on the class describing the expected lifetime of this class?

…TopicIds

squah-confluent · 2024-10-02T18:39:16Z

@mumrah

Seeing that the cache is not actually used outside of tests and benchmarks, I'm guessing this is still WIP.

It's used in TargetAssignmentBuilder.build(), which is used by the new group coordinator.

I've updated the javadoc to describe the lifetime of the cache.

dajac

LGTM, thanks.

dajac · 2024-10-03T07:39:43Z

@mumrah I will merge it. If you have further comments, @squah-confluent can address separately.

MINOR: Cache topic resolution in TopicIds set

165ee41

Looking up topics in a TopicsImage is relatively slow. Cache the results in TopicIds to improve assignor performance.

github-actions bot added the performance label Sep 26, 2024

dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Sep 26, 2024

dajac self-requested a review September 26, 2024 13:53

dajac mentioned this pull request Sep 26, 2024

(WIP) MINOR: Cache topic resolution in TopicIds set #16527

Closed

3 tasks

dajac reviewed Sep 26, 2024

View reviewed changes

FrankYang0529 and others added 24 commits September 30, 2024 17:19

MINOR: fix failed cases in FeatureCommandTest (apache#17287)

a59e8c5

Reviewers: David Arthur <[email protected]>

KAFKA-16792: Enable consumer unit tests that fail to fetch offsets on…

03b4cdc

…ly for new consumer with poll(0) (apache#16982) Reviewers: Lianet Magrans <[email protected]>, TaiJuWu <[email protected]>, Kirk True <[email protected]>, TengYao Chi <[email protected]>

KAFKA-17563 Move RequestConvertToJson to server module (apache#17223)

25b048e

Reviewers: Chia-Ping Tsai <[email protected]>

KAFKA-17154: New consumer subscribe may join group without a call to …

0d8485a

…consumer.poll (apache#17165) Reviewers: Lianet Magrans <[email protected]>, TaiJuWu <[email protected]>

KAFKA-16683 Extract security-related helpers from scala.TestUtils to …

5aaf623

…java class (apache#16912) Reviewers: Chia-Ping Tsai <[email protected]>

MINOR: Remove duplicate method (apache#17291)

813d79c

Reviewers: Chia-Ping Tsai <[email protected]>

KAFKA-17488: Cleanup (test) code for Kafka Streams "metric version" (a…

0589e93

…pache#17182) This PR simply StreamsMetricsImpl to avoid passing in the unused "metric version" parameter. Reviewers: Matthias J. Sax <[email protected]>

KAFKA-17626: Move common fetch related classes from storage to server…

3b555b8

…-common (apache#17289) Reviewers: Chia-Ping Tsai <[email protected]>, David Arthur <[email protected]>

MINOR: Set default group.consumer.migration.policy to BIDIRECTIONAL (a…

2df3ff0

…pache#17295) This is a small patch to change the default value of group.consumer.migration.policy to BIDIRECTIONAL. Reviewers: David Jacot <[email protected]>

MINOR: Fix a race and add JMH bench for HdrHistogram (apache#17221)

945023d

KAFKA-17581: AsyncKafkaConsumer can't unsubscribe invalid topics (apa…

f0a2d43

…che#17244) Reviewers: Kirk True <[email protected]>, Lianet Magrans <[email protected]>

KAFKA-17619: Remove zk type and instance from ClusterTest (apache#17284)

f438bb4

Reviewers: Colin P. McCabe <[email protected]>, Chia-Ping Tsai <[email protected]>, David Arthur <[email protected]>

KAFKA-14572: Migrate EmbeddedKafkaCluster used by Streams integration…

b905a9f

… tests from EmbeddedZookeeper to KRaft (apache#17016) Migrate the EmbeddedKafkaCluster from the EmbeddedZookeeper to KRaft Reviewers Bill Bejeck <[email protected]>

KAFKA-17636 Fix missing SCRAM bootstrap records (apache#17305)

d9254d5

Fixes a regression introduced by apache#16669 which inadvertently stopped processing SCRAM arguments from kafka-storage.sh Reviewers: Colin P. McCabe <[email protected]>, Federico Valeri <[email protected]>

MINOR Don't run tests for Draft PRs (apache#17299)

0b97fa6

Reviewers: Chia-Ping Tsai <[email protected]>

KAFKA-6197: Update Streams API and Javadoc references in documentation (

1f9c7ae

apache#17215) Reviewers: Matthias J. Sax <[email protected]>

fixup: add toString implementation to TopicResolvers

e94ed0f

fixup: make BaseTopicResolver private

7281eb2

github-actions bot added storage Pull requests that target the storage module KIP-932 Queues for Kafka build Gradle build or GitHub Actions transactions Transactions and EOS clients labels Sep 30, 2024

squah-confluent added 3 commits September 30, 2024 21:04

Merge remote-tracking branch 'origin/trunk' into squah-cache-topicids…

f5d98e0

…-topic-resolution

Merge pull request #135 from confluentinc/squah-cache-topicids-topic-…

7e718fb

…resolution Updates for apache#17285

squah-confluent requested a review from dajac September 30, 2024 20:12

mumrah removed streams core Kafka Broker consumer tools storage Pull requests that target the storage module KIP-932 Queues for Kafka build Gradle build or GitHub Actions transactions Transactions and EOS clients labels Oct 1, 2024

mumrah reviewed Oct 1, 2024

View reviewed changes

mumrah added the ci-approved label Oct 1, 2024

squah-confluent added 2 commits October 2, 2024 19:34

fixup: remove AbstractTopicResolver, inline equals and hashCode into …

3c71192

…TopicIds

fixup: add comment about lifetime of CachedTopicResolver

43a881f

squah-confluent requested a review from mumrah October 2, 2024 18:39

dajac approved these changes Oct 3, 2024

View reviewed changes

dajac merged commit 99e1d8f into apache:trunk Oct 3, 2024
9 checks passed

dajac deleted the squah-cache-topicids-topic-resolution branch October 3, 2024 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Cache topic resolution in TopicIds set #17285

MINOR: Cache topic resolution in TopicIds set #17285

squah-confluent commented Sep 26, 2024 •

edited

Loading

dajac left a comment

dajac Sep 26, 2024

squah-confluent Sep 28, 2024

dajac Sep 26, 2024

squah-confluent Sep 28, 2024

mumrah left a comment

mumrah Oct 1, 2024

squah-confluent Oct 2, 2024

dajac commented Oct 2, 2024

mumrah commented Oct 2, 2024

squah-confluent commented Oct 2, 2024 •

edited

Loading

dajac left a comment

dajac commented Oct 3, 2024

MINOR: Cache topic resolution in TopicIds set #17285

MINOR: Cache topic resolution in TopicIds set #17285

Conversation

squah-confluent commented Sep 26, 2024 • edited Loading

Committer Checklist (excluded from commit message)

dajac left a comment

Choose a reason for hiding this comment

dajac Sep 26, 2024

Choose a reason for hiding this comment

squah-confluent Sep 28, 2024

Choose a reason for hiding this comment

dajac Sep 26, 2024

Choose a reason for hiding this comment

squah-confluent Sep 28, 2024

Choose a reason for hiding this comment

mumrah left a comment

Choose a reason for hiding this comment

mumrah Oct 1, 2024

Choose a reason for hiding this comment

squah-confluent Oct 2, 2024

Choose a reason for hiding this comment

dajac commented Oct 2, 2024

mumrah commented Oct 2, 2024

squah-confluent commented Oct 2, 2024 • edited Loading

dajac left a comment

Choose a reason for hiding this comment

dajac commented Oct 3, 2024

squah-confluent commented Sep 26, 2024 •

edited

Loading

squah-confluent commented Oct 2, 2024 •

edited

Loading