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
