msn-tldr commented on code in PR #14564:
URL: https://github.com/apache/kafka/pull/14564#discussion_r1370368421
##########
clients/src/main/java/org/apache/kafka/clients/MetadataCache.java:
##########
@@ -150,7 +150,7 @@ MetadataCache mergeWith(String newClusterId,
// We want the most recent topic ID. We start with the previous ID
stored for retained topics and then
// update with newest information from the MetadataResponse. We always
take the latest state, removing existing
// topic IDs if the latest state contains the topic name but not a
topic ID.
- Map<String, Uuid> newTopicIds = topicIds.entrySet().stream()
+ Map<String, Uuid> newTopicIds = this.topicIds.entrySet().stream()
Review Comment:
Net effect of the bug was that in the merged cache, the IDs of retained
topics(from pre-existing metadata) would be lost in the newly built cache(via
merging).
As the current comment explains the intention of the code is to get merged
list of topic-ids.
> We start with the previous ID stored for retained topics and then update
with newest information from the MetadataResponse.
This should be done by initialising the merged list of topic-ids with
retained topic(`this.topicIds`). And then updating with newest
information(`topicIds`). But the code uses `topicIds` even to get retained
topic-ids. The bug got introduced inadvertently
[here](https://github.com/apache/kafka/pull/11004/files#diff-5e4d9713b6e5a386e5cac7e81215160948859243a71b439c3a990876e56b3ec5).
Now it has a test to catch regressions in behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
