yashmayya commented on code in PR #14024:
URL: https://github.com/apache/kafka/pull/14024#discussion_r1266957728


##########
connect/api/src/main/java/org/apache/kafka/connect/sink/SinkRecord.java:
##########
@@ -65,7 +180,8 @@ public SinkRecord newRecord(String topic, Integer 
kafkaPartition, Schema keySche
     @Override
     public SinkRecord newRecord(String topic, Integer kafkaPartition, Schema 
keySchema, Object key, Schema valueSchema, Object value,
                                 Long timestamp, Iterable<Header> headers) {
-        return new SinkRecord(topic, kafkaPartition, keySchema, key, 
valueSchema, value, kafkaOffset(), timestamp, timestampType, headers);
+        return new SinkRecord(topic, kafkaPartition, keySchema, key, 
valueSchema, value, kafkaOffset(), timestamp, timestampType, headers,
+                originalTopic(), originalKafkaPartition(), 
originalKafkaOffset());

Review Comment:
   Haha wow I completely glossed over that, nice catch.
   
   > It'd be great if we could fix that bug (and possibly add testing coverage 
for it)
   
   I think it might be worth filing a separate bug ticket and PR to fix that so 
that we could also backport it as well? We did discuss whether or not the 
method is even useful 
[here](https://github.com/apache/kafka/pull/14024#discussion_r1265694650), but 
given that we technically do support such use cases (and can't prove that it 
isn't used), it'd probably be good to backport a fix for such a bad bug if 
possible. WDYT?
   
   > And IMO this is a decent reason to abandon using methods to differentiate 
between fields and parameters
   
   Yeah, I'm definitely convinced since I missed it as well 😆



##########
connect/api/src/main/java/org/apache/kafka/connect/sink/SinkRecord.java:
##########
@@ -65,7 +180,8 @@ public SinkRecord newRecord(String topic, Integer 
kafkaPartition, Schema keySche
     @Override
     public SinkRecord newRecord(String topic, Integer kafkaPartition, Schema 
keySchema, Object key, Schema valueSchema, Object value,
                                 Long timestamp, Iterable<Header> headers) {
-        return new SinkRecord(topic, kafkaPartition, keySchema, key, 
valueSchema, value, kafkaOffset(), timestamp, timestampType, headers);
+        return new SinkRecord(topic, kafkaPartition, keySchema, key, 
valueSchema, value, kafkaOffset(), timestamp, timestampType, headers,
+                originalTopic(), originalKafkaPartition(), 
originalKafkaOffset());

Review Comment:
   Haha wow I completely glossed over that, nice catch.
   
   > It'd be great if we could fix that bug (and possibly add testing coverage 
for it)
   
   I think it might be worth filing a separate bug ticket and PR to fix that so 
that we could also backport it as well? We did discuss whether or not the 
method is even useful 
[here](https://github.com/apache/kafka/pull/14024#discussion_r1265694650), but 
given that we technically do support such use cases (and can't prove that it 
isn't used), it'd probably be good to backport a fix for such a bad bug if 
possible. WDYT?
   
   > And IMO this is a decent reason to abandon using methods to differentiate 
between fields and parameters
   
   Yeah, I'm definitely convinced since I missed it as well 😆



-- 
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

Reply via email to