jolshan commented on code in PR #14444: URL: https://github.com/apache/kafka/pull/14444#discussion_r1350932166
########## clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java: ########## @@ -67,20 +69,31 @@ public ProduceResponse(ProduceResponseData produceResponseData) { */ @Deprecated public ProduceResponse(Map<TopicPartition, PartitionResponse> responses) { - this(responses, DEFAULT_THROTTLE_TIME); + this(responses, DEFAULT_THROTTLE_TIME, Collections.emptyList()); } /** - * Constructor for the latest version + * Constructor for versions <= 9 * @param responses Produced data grouped by topic-partition * @param throttleTimeMs Time in milliseconds the response was throttled */ @Deprecated public ProduceResponse(Map<TopicPartition, PartitionResponse> responses, int throttleTimeMs) { - this(toData(responses, throttleTimeMs)); + this(toData(responses, throttleTimeMs, Collections.emptyList())); + } + + /** + * Constructor for the latest version + * @param responses Produced data grouped by topic-partition + * @param throttleTimeMs Time in milliseconds the response was throttled + * @param nodeEndpoints List of node endpoints + */ + @Deprecated Review Comment: KAFKA-10730 is a pretty dormant JIRA. I do agree that there is some level of conversion. I wonder if folks have a strong opinion about this conversion still. Looking into this further, I see the change would need to be made to appendRecords and the ProducePartitionStatus. It doesn't look too crazy, but also understandable this is not the scope for this PR. I wonder if KAFKA-9682 was premature in deprecating the constructor. I guess our options are leaving it deprecated and adding a deprecated method or removing the deprecation until KAFKA-10730 is completed. (I almost just want to fix it so this doesn't happen in the future 😂 ) ########## clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java: ########## @@ -67,20 +69,31 @@ public ProduceResponse(ProduceResponseData produceResponseData) { */ @Deprecated public ProduceResponse(Map<TopicPartition, PartitionResponse> responses) { - this(responses, DEFAULT_THROTTLE_TIME); + this(responses, DEFAULT_THROTTLE_TIME, Collections.emptyList()); } /** - * Constructor for the latest version + * Constructor for versions <= 9 * @param responses Produced data grouped by topic-partition * @param throttleTimeMs Time in milliseconds the response was throttled */ @Deprecated public ProduceResponse(Map<TopicPartition, PartitionResponse> responses, int throttleTimeMs) { - this(toData(responses, throttleTimeMs)); + this(toData(responses, throttleTimeMs, Collections.emptyList())); + } + + /** + * Constructor for the latest version + * @param responses Produced data grouped by topic-partition + * @param throttleTimeMs Time in milliseconds the response was throttled + * @param nodeEndpoints List of node endpoints + */ + @Deprecated Review Comment: KAFKA-10730 is a pretty dormant JIRA. I do agree that there is some level of conversion. I wonder if folks have a strong opinion about this conversion still. Looking into this further, I see the change would need to be made to appendRecords and the ProducePartitionStatus. It doesn't look too crazy, but also understandable this is not the scope for this PR. I wonder if KAFKA-9682 was premature in deprecating the constructor. I guess our options are leaving it deprecated and adding a deprecated method or removing the deprecation until KAFKA-10730 is completed. (I almost just want to fix it so this doesn't happen in the future 😂 ) -- 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