KAFKA-15216: InternalSinkRecord::newRecord should not ignore new headers

[yashmayya]

• https://issues.apache.org/jira/browse/KAFKA-15216

• kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/InternalSinkRecord.java

  Lines 50 to 56 in a1f6ab6

  	@Override
  	public SinkRecord newRecord(String topic, Integer kafkaPartition, Schema keySchema, Object key,
  	Schema valueSchema, Object value, Long timestamp,
  	Iterable<Header> headers) {
  	return new InternalSinkRecord(originalRecord, topic, kafkaPartition, keySchema, key,
  	valueSchema, value, kafkaOffset(), timestamp, timestampType(), headers());
  	}

  
  The headers argument passed to the InternalSinkRecord constructor is the instance field via the accessor headers() method instead of the newRecord method's headers argument value (originally discovered here.).

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[C0urante]

The change from private to protected technically counts as a change to public interface, so we'd need a KIP for that. I'm also a little hesitant to upgrade the visibility of these members regardless since that would limit the compatibility of plugins that rely on them (most likely by subclassing ConnectRecord, SinkRecord, etc.), since that would render them binary incompatible with older versions of Connect where the fields were still private.

Can we reduce the scope here to use fields instead of methods wherever possible, but without altering the visibility of any parts of our public API?

[yashmayya]

The change from private to protected technically counts as a change to public interface, so we'd need a KIP for that

Ah, I did wonder about this but wasn't entirely certain, thanks for the clarification!

I'm also a little hesitant to upgrade the visibility of these members regardless since that would limit the compatibility of plugins that rely on them (most likely by subclassing ConnectRecord, SinkRecord, etc.), since that would render them binary incompatible with older versions of Connect where the fields were still private.

I considered the binary compatibility impact of this change directly on plugins themselves (and there shouldn't be any), but good point on the backward compatibility restriction that would be imposed on any potential external subclasses of ConnectRecord due to this change.

Can we reduce the scope here to use fields instead of methods wherever possible, but without altering the visibility of any parts of our public API?

Done

[C0urante]

Thanks Yash! LGTM, will merge pending CI build.