jolshan commented on a change in pull request #9944:
URL: https://github.com/apache/kafka/pull/9944#discussion_r592761061
##########
File path:
clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java
##########
@@ -80,15 +156,34 @@ public Errors error() {
return Errors.forCode(data.errorCode());
}
- public LinkedHashMap<TopicPartition, FetchResponseData.PartitionData>
responseData() {
+ public LinkedHashMap<TopicPartition, FetchResponseData.PartitionData>
responseData(Map<Uuid, String> topicNames, short version) {
+ if (version < 13)
+ return toResponseDataMap();
+ return toResponseDataMap(topicNames);
+
+ }
+
+ // TODO: Should be replaced or cleaned up. The idea is that in KafkaApis
we need to reconstruct responseData even though we could have just passed in
and out a map.
+ // With topic IDs, recreating the map takes a little more time since we
have to get the topic name from the topic ID to name map.
+ // The refactor somewhat helps in KafkaApis, but we have to recompute the
map instead of just returning it.
+ // This is unsafe in test cases now (FetchSessionTest) where it used to
be safe. Before it would just pull the responseData map.
+ // If we wanted to recompute with topicNames we could call
responseData(topicNames) however,
+ // now it will just return the version computed here.
+
+ // Used when we can guarantee responseData is populated with all possible
partitions
+ // This occurs when we have a response version < 13 or we built the
FetchResponse with
+ // responseDataMap as a parameter and we have the same topic IDs available.
+ public LinkedHashMap<TopicPartition, FetchResponseData.PartitionData>
resolvedResponseData() {
Review comment:
Yeah, so for example, TopicPartitions are given to
ReplicaManager.fetchMessages where it is used heavily. I think we still want
the map for now.
One thing I was confused about with this code is why we generate
`unconvertedFetchResponse` with `updateAndGenerateResponseData`. We pass in the
map to create `unconvertedFetchResponse`. With the optimization to lazily
compute the map, recompute it a few lines of code later in `createResponse`.
I'm hoping to avoid this somehow. (I think the optimization is great everywhere
else though and I was heading in that direction originally with this PR)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
