KAFKA-7273: extend Connect Converter to support headers by sap1ens · Pull Request #6362 · apache/kafka

sap1ens · 2019-03-04T04:35:01Z

Update: See KIP-440, approved.

Extending Converter interface to support headers in backwards-compatible way. Very similar to Serializer and Deserializer interfaces.

A few questions I have to the contributors:

Do I need to update internal components like KafkaStatusBackingStore that use Converters? It doesn't look like they use headers in the moment, so I don't think it's necessary.
How about other Converter implementations like JsonConverter? Should they all be switched to the new interface method even if they don't use the headers?
~~Is KIP required in this case? Converter is a public interface, but the change is trivial and backwards-compatible.~~ Created

Committer Checklist (excluded from commit message)

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

ryannedolan

I don't think this change makes sense, and also duplicates an existing API. See HeaderConverter and KIP-145.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-145+-+Expose+Record+Headers+in+Kafka+Connect

sap1ens · 2019-03-04T21:18:36Z

I don't think this change makes sense, and also duplicates an existing API. See HeaderConverter and KIP-145.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-145+-+Expose+Record+Headers+in+Kafka+Connect

HeaderConverter handles use-case when you need to transform headers. I don't want to transform them, I simply want to get access to the headers in my converter, together with the payload.

sap1ens · 2019-03-05T05:19:22Z

Hey @ewencp, @hachikuji, any chance you can take a look? Thanks

ewencp

Left one (major) comment. Didn't review rest in detail, but most looks straightforward and reasonable -- with the caveat that this particular direction warrants much broader review.

ewencp · 2019-03-05T06:48:50Z

@sap1ens regarding KIPs:

I agree, I think your use case might be different from HeaderConverter and an interface to handle multiple components of the message together could have value. HeaderConverter was intended to extend existing "isolated" converters, but may not have accounted for interactions between headers and keys/values.
This does require a KIP. At the potential cost of formality and overhead, we prefer to be very careful about public interfaces. "simple and backwards compatible" isn't sufficient to not require review -- a backwards compatible addition is still a commitment of support moving forward and a limitation on evolvability. (<-- is just stating rules, not any sort of "no" or discouragement from making the change! the KIP process just makes us thoroughly evaluate and think through the implications of a change like this)

Many folks are happy to help guide through KIP process (which can be pretty easy in some cases, but tbh I suspect this case will require some significant discussion and thought!). Off the top of my head, @kkonstantine @rhauch @wicknicks @rayokota can probably review and help with guidance on KIP and design.

ryannedolan · 2019-03-05T18:15:49Z

Suggest adding unit tests to ensure headers are preserved.

sap1ens · 2019-03-12T03:46:14Z

Created KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers

sap1ens · 2019-03-24T17:06:13Z

@ewencp @kkonstantine @rhauch @wicknicks @rayokota any chance you help with the KIP review for this? Discussion thread has been created https://lists.apache.org/thread.html/1fc1e3d2cddd8311d3db7c98f0d09a1a137ca4b20d1f3c8ab203a855@%3Cdev.kafka.apache.org%3E

sap1ens · 2019-08-22T22:44:52Z

KIP has been accepted. I've pushed another commit with extra tests.

ryannedolan

I'd like to see a unit tests that covers the scenario described as motivation in the KIP, where serde is influenced by some header field (rather than just passed through).

rhauch

Looks pretty good, though I have a few comments / suggestions below.

sap1ens · 2019-08-26T01:03:57Z

@ryannedolan @rhauch thanks for the feedback so far, I've addressed it with commit 009212c

sap1ens · 2019-08-26T02:48:21Z

I've also had to suppress ClassFanOutComplexity check for WorkerSinkTaskTest/WorkerSourceTaskTest files. They are pretty big and test a lot of different use-cases. Splitting into different test files can help, but it's a big and not very obvious change.

sap1ens · 2019-09-04T03:37:39Z

@ryannedolan @rhauch hi, just wanted to check if you have time to take a look at the latest changes, thanks!

ryannedolan

thanks for the improved tests, lgtm

rhauch

Overall this is really good. My only concern is the use of Java serialization, which would be good to avoid because of the security implications, even if it's only used in test code.

rhauch · 2019-09-18T12:50:52Z

@sap1ens yes, this looks good. Please resolve conflicts and ping me again. Thanks!

# Conflicts: # connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java

sap1ens · 2019-09-20T21:56:34Z

@rhauch merge conflict has been resolved, this PR is ready.

sap1ens · 2019-09-21T03:28:34Z

Failed test seems to be completely unrelated (flaky integration test perhaps?):

kafka.api.AdminClientIntegrationTest.testElectUncleanLeadersAndNoop

rhauch · 2019-09-25T16:02:39Z

This PR does not change all of the uses of the older Converter methods:

JsonConverter and its tests
StringConverter and its tests
ByteArrayConverter and its tests
NumberConverter and its tests
KafkaConfigBackingStore and its tests
KafkaStatusBackingStore and its tests
OffsetStorageReaderImpl
OffsetStorageWriter and its tests

However, I think this is perfectly acceptable for a couple of reasons: changing those would increase the size of this PR, while leaving them as-is helps verify backward compatibility of converters that only implement the older methods.

The PR does change the important call sites in WorkerSinkTask and WorkerSourceTask, which are the primary places where converters are used (except for the internal topic records in the storage-related classes that should always use the JsonConverter that doesn't need the new methods).

rhauch

LGTM. Thanks, @sap1ens!

Conflicts: * .gitignore: addition of clients/src/generated-test was near local additions for support-metrics. * checkstyle/suppressions.xml: upstream refactoring of exclusions for generator were near the local changes for support-metrics. * gradle.properties: scala version bump caused a minor conflict due to the kafka version change locally. gradle/dependencies.gradle: bcpkix version bump was near avro additions in the local version. * apache-github/trunk: (49 commits) KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353) MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394) MINOR:fixed typo and removed outdated varilable name (apache#7402) KAFKA-8934: Create version file during build for Streams (apache#7397) KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382) KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309) KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380) MINOR: Address review comments for KIP-504 authorizer changes (apache#7379) MINOR: add versioning to request and response headers (apache#7372) KAFKA-7273: Extend Connect Converter to support headers (apache#6362) MINOR: improve the Kafka RPC code generator (apache#7340) MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344) KAFKA-8880: Docs on upgrade-guide (apache#7385) KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321) KAFKA-8580: Compute RocksDB metrics (apache#7263) KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304) HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363) KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374) MINOR: remove unnecessary null check (apache#7299) KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413) ...

ryannedolan suggested changes Mar 4, 2019

View reviewed changes

Comment thread connect/api/src/main/java/org/apache/kafka/connect/storage/Converter.java Outdated

Comment thread connect/api/src/main/java/org/apache/kafka/connect/storage/Converter.java Outdated

ewencp reviewed Mar 5, 2019

View reviewed changes

Comment thread connect/api/src/main/java/org/apache/kafka/connect/storage/Converter.java Outdated

sap1ens changed the title ~~KAFKA-7273: [WIP] extend Connect Converter to support headers~~ KAFKA-7273: extend Connect Converter to support headers Mar 12, 2019

KAFKA-7273: extend Connect Converter to support headers

3d019fa

sap1ens force-pushed the add-headers-to-converters branch from a968e91 to 3d019fa Compare August 22, 2019 04:22

KAFKA-7273: add more tests

2d3302b

ryannedolan approved these changes Aug 22, 2019

View reviewed changes

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSinkTaskTest.java

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java Outdated

rhauch reviewed Aug 25, 2019

View reviewed changes

KAFKA-7273: addressing feedback, adding more tests

009212c

sap1ens added 2 commits August 25, 2019 18:16

KAFKA-7273: minor tweaks

368cc61

KAFKA-7273: fix linting errors

f732fc6

ryannedolan approved these changes Sep 4, 2019

View reviewed changes

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSinkTaskTest.java Outdated

rhauch reviewed Sep 9, 2019

View reviewed changes

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/TestConverterWithHeaders.java Outdated

KAFKA-7273: addressing feedback, tweaking tests

993966a

Merge branch 'trunk' into add-headers-to-converters

4b91ff4

# Conflicts: # connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSourceTaskTest.java

rhauch approved these changes Sep 25, 2019

View reviewed changes

rhauch merged commit 70d1bb4 into apache:trunk Sep 25, 2019

kkonstantine added the connect label Oct 16, 2020

Conversation

sap1ens commented Mar 4, 2019 • edited by rhauch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ryannedolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sap1ens commented Mar 4, 2019

Uh oh!

sap1ens commented Mar 5, 2019

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ewencp commented Mar 5, 2019

Uh oh!

ryannedolan commented Mar 5, 2019

Uh oh!

sap1ens commented Mar 12, 2019

Uh oh!

sap1ens commented Mar 24, 2019

Uh oh!

sap1ens commented Aug 22, 2019

Uh oh!

ryannedolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sap1ens commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sap1ens commented Aug 26, 2019

Uh oh!

sap1ens commented Sep 4, 2019

Uh oh!

ryannedolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhauch commented Sep 18, 2019

Uh oh!

sap1ens commented Sep 20, 2019

Uh oh!

sap1ens commented Sep 21, 2019

Uh oh!

rhauch commented Sep 25, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sap1ens commented Mar 4, 2019 •

edited by rhauch

Loading

sap1ens commented Aug 26, 2019 •

edited

Loading