-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ErrantRecordReporter support #57
Add ErrantRecordReporter support #57
Conversation
48d4b30
to
5c333f1
Compare
Related to #34 |
Hi @CodeLikeAGecko, thanks for your contribution. |
@AnatolyPopov Can you point me to some documentation on how to set up the integration test environment? I am not finding anything, but may have missed it. My environment is that I develop on a remote linux vm. If no documentation, some basic instructions or an example elsewhere on the Internet would be very helpful to me. Thanks! |
@CodeLikeAGecko Sure! The only thing you need is Docker. If you have Docker installed then |
Add errors.tolerance and ErrantRecordReporter support. Updating to support kafkaVersion 2.6 which is when ErrantRecordReporter became available in Kafka connect. Can also add the following parameters to the http-sink properties file to handle messages that are reported to the ErrantRecordReporter. errors.deadletterqueue.topic.name errors.deadletterqueue.context.headers.enable errors.deadletterqueue.topic.replication.factor Preserving backwards compatibility on batch send by only using ErrantRecordReporting if non-batch.
Validate that errors.tolerance=all and batching.enabled=true are not both set. Test that a config exception is thrown if both set.
Updating Kafka to version 2.8 and all gradle dependencies and integration tests to match.
5c333f1
to
e2c83ed
Compare
e2c83ed
to
984a7ba
Compare
@AnatolyPopov Sorry for the long delay. It took me a while to figure things out and my regular job was keeping me busy. But it is hopefully ready now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Here are some comments.
src/integration-test/java/io/aiven/kafka/connect/http/mockserver/FailingPeriodicHandler.java
Outdated
Show resolved
Hide resolved
src/test/java/io/aiven/kafka/connect/http/config/HttpSinkConfigTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/SchemaRegistryContainer.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/IntegrationTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/JsonIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/AvroIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/ErrantRecordTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/ErrantRecordTest.java
Outdated
Show resolved
Hide resolved
src/integration-test/java/io/aiven/kafka/connect/http/mockserver/FailingPeriodicHandler.java
Outdated
Show resolved
Hide resolved
Changed JUnit assertions to AssertJ and fixed failing tests. Other changes as requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments for things that I missed before.
src/integration-test/java/io/aiven/kafka/connect/http/AvroIntegrationTest.java
Outdated
Show resolved
Hide resolved
Now tests with Kafka version 2.6.3. Note that the related Confluent Platform introduced a dependancy issue with v6.0.3. The tests fail with a NoSuchMethodError because the Jersey server changed a method signature in its version 2.34 and that is used by Confluent v6.0.3. The rest of the system is using Jersey server 2.31.
@AnatolyPopov Thanks for finding/pointing out these issues. As you will see in the commit message I had some difficulty with a dependency issue that turned out to have been introduced in Confluent Platform v6.0.3. It isn't an area I am very knowledgeable in but it looks like Confluent 6.0.3 started using the jersey-common v2.34 jar file for something. The rest of the test ecosystem was using Jersey 2.31 jar files that was brought in by Kafka version 2.6.3. I think the Confluent-built jar was resolved for a call by the Kafka-related stuff and a method signature had changed, leading to a java.lang.NoSuchMethodError failure in the integration tests. Back in March I couldn't figure it out other than going to the latest Kafka 2.X. This time I was more knowledgeable and isolated it to the Confluent Platform version 6.0.3 allowing Kafka 2.6 to be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this dependency-related thing a bit more precisely today so a couple more comments. Hopefully the last thing. Thanks for your patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we clarify if the license is compatible, I'll like to hold this PR
Updating versions to better align dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your contribution!
Add errors.tolerance and ErrantRecordReporter support.
Updating to support kafkaVersion 2.6 which is when
ErrantRecordReporter became available in Kafka connect. Can
also add the following parameters to the http-sink properties
file to handle messages that are reported to the
ErrantRecordReporter.
errors.deadletterqueue.topic.name
errors.deadletterqueue.context.headers.enable
errors.deadletterqueue.topic.replication.factor
Preserving backwards compatibility on batch send by only using
ErrantRecordReporting if non-batch.