C0urante commented on code in PR #14024: URL: https://github.com/apache/kafka/pull/14024#discussion_r1266917499
########## 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: There's a bug in the [third example](https://github.com/apache/kafka/blob/a1f6ab69387deb10988461152a0087f0cd2827c4/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/InternalSinkRecord.java#L54-L55) 😱 The `newRecord` method accepts an `Iterable<Header> headers` parameter, but then when constructing the `InternalSinkRecord` that's used as the return value, it passes in `headers()` instead of `headers` as the parameter for the new record's headers, which causes the headers that were supplied to the `newRecord` method to be ignored. It'd be great if we could fix that bug (and possibly add testing coverage for it). And IMO this is a decent reason to abandon using methods to differentiate between fields and parameters, though as long as there aren't any bugs in this PR I could live with it if you feel strongly that it's still worth making that distinction. ########## 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: There's a bug in the [third example](https://github.com/apache/kafka/blob/a1f6ab69387deb10988461152a0087f0cd2827c4/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/InternalSinkRecord.java#L54-L55) 😱 The `newRecord` method accepts an `Iterable<Header> headers` parameter, but then when constructing the `InternalSinkRecord` that's used as the return value, it passes in `headers()` instead of `headers` as the parameter for the new record's headers, which causes the headers that were supplied to the `newRecord` method to be ignored. It'd be great if we could fix that bug (and possibly add testing coverage for it). And IMO this is a decent reason to abandon using methods to differentiate between fields and parameters, though as long as there aren't any bugs in this PR I could live with it if you feel strongly that it's still worth making that distinction. -- 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