KAFKA-18583 Fix getPartitionReplicaEndpoints for KRaft #18635
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Although MetadataCache's getPartitionReplicaEndpoints takes a single topic-partition, the KRaftMetadataCache implementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions. Note that this is not a recent regression - it has been a part of KRaftMetadataCache since its creation.
github-actions
bot
added
triage
dajac
reviewed
Jan 20, 2025
Comment on lines
569
to
572
| // Verify that for each partition we have exactly $replicationFactor endpoints | ||
| assertEquals(replicationFactor, | ||
| replicaSet.size, | ||
| s"Unexpected replica set $replicaSet for partition $partitionId") |
There was a problem hiding this comment.
Would it be possible to also verify that the method returns the replicas that we expect (replica ids + nodes)?
There was a problem hiding this comment.
Fixed, let me know if there's anything you'd like changed in the chosen approach.
ijuma
reviewed
Jan 21, 2025
| @@ -433,7 +433,7 @@ class KRaftMetadataCache( | |||
| val image = _currentImage | |||
| val result = new mutable.HashMap[Int, Node]() | |||
| Option(image.topics().getTopic(tp.topic())).foreach { topic => | |||
| topic.partitions().values().forEach { partition => | |||
| Option(topic.partitions().get(tp.partition())).foreach { partition => | |||
There was a problem hiding this comment.
Ouch - this is a painful oversight.
ijuma
reviewed
Jan 21, 2025
| @@ -511,6 +512,101 @@ class MetadataCacheTest { | |||
| assertEquals(initialBrokerIds.toSet, aliveBrokersFromCache.map(_.id).toSet) | |||
| } | |||
|
|
|||
| @ParameterizedTest | |||
| @MethodSource(Array("cacheProvider")) | |||
| def testGetPartitionReplicaEndpoints(cache: MetadataCache): Unit = { | |||
There was a problem hiding this comment.
Does this mean that we didn't have a test for this method?
There was a problem hiding this comment.
Mostly - there is an FFF integration test verifying that the method doesn't return a shutdown node (eventually).
dajac
pushed a commit
that referenced
this pull request
Jan 21, 2025
Although `MetadataCache`'s `getPartitionReplicaEndpoints` takes a single topic-partition, the `KRaftMetadataCache` implementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions. Note that this is not a recent regression - it has been a part of `KRaftMetadataCache` since its creation. Reviewers: Ismael Juma <[email protected]>, David Jacot <[email protected]>
|
Merged it to trunk and to 4.0. @dimitarndimitrov I was not able to cherry-pick it to 3.9 and to 3.8 due to conflicts. Could you please open PRs for those branches? |
dimitarndimitrov
added a commit
to dimitarndimitrov/kafka
that referenced
this pull request
Jan 21, 2025
The cherry-pick required reimplementing the accompanying test to work with UpdateMetadataRequest (removed in 4.0 and trunk) in order to also apply to `ZkMetadataCache`. If the removal of UpdateMetadataRequest is backported here as well, the test can be changed to match the trunk version. Conflicts: core/src/test/scala/unit/kafka/server/MetadataCacheTest.scala
Hey @dajac check out #18657 when you have a moment. The sole commit in that PR also applies cleanly to 3.8 (tested locally). |
pranavt84
pushed a commit
to pranavt84/kafka
that referenced
this pull request
Jan 27, 2025
Although `MetadataCache`'s `getPartitionReplicaEndpoints` takes a single topic-partition, the `KRaftMetadataCache` implementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions. Note that this is not a recent regression - it has been a part of `KRaftMetadataCache` since its creation. Reviewers: Ismael Juma <[email protected]>, David Jacot <[email protected]>
airlock-confluentinc bot
pushed a commit
to confluentinc/kafka
that referenced
this pull request
Jan 27, 2025
Although `MetadataCache`'s `getPartitionReplicaEndpoints` takes a single topic-partition, the `KRaftMetadataCache` implementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions. Note that this is not a recent regression - it has been a part of `KRaftMetadataCache` since its creation. Reviewers: Ismael Juma <[email protected]>, David Jacot <[email protected]>
manoj-mathivanan
pushed a commit
to manoj-mathivanan/kafka
that referenced
this pull request
Feb 19, 2025
Although `MetadataCache`'s `getPartitionReplicaEndpoints` takes a single topic-partition, the `KRaftMetadataCache` implementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions. Note that this is not a recent regression - it has been a part of `KRaftMetadataCache` since its creation. Reviewers: Ismael Juma <[email protected]>, David Jacot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although
MetadataCache'sgetPartitionReplicaEndpointstakes a single topic-partition, theKRaftMetadataCacheimplementation iterates over all partitions of the matching topic. This is not necessary and can cause significant performance degradation when the topic has a relatively high number of partitions.Note that this is not a recent regression - it has been a part of
KRaftMetadataCachesince its creation.Committer Checklist (excluded from commit message)