Skip to content
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

Merged

Conversation

CodeLikeAGecko
Copy link
Contributor

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.

@CodeLikeAGecko CodeLikeAGecko requested a review from a team as a code owner February 7, 2022 17:51
@CodeLikeAGecko CodeLikeAGecko force-pushed the http-sink-errors-tolerance branch from 48d4b30 to 5c333f1 Compare February 10, 2022 21:34
@ivanyu
Copy link
Collaborator

ivanyu commented Feb 21, 2022

Related to #34

@AnatolyPopov
Copy link
Contributor

Hi @CodeLikeAGecko, thanks for your contribution.
Could you rebase the branch on top of the current main? We just enabled integration test in GitHub actions and seems they are failing with your changes. And could you add some more integration tests with dead letter queue covering your changes?

@CodeLikeAGecko
Copy link
Contributor Author

@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!

@AnatolyPopov
Copy link
Contributor

@CodeLikeAGecko Sure! The only thing you need is Docker. If you have Docker installed then ./gradlew integrationTest should run the tests(unless you have any custom Docker configuration). Basically, integration tests are implemented using Testcontainers library. You can use test from src/main/integrationTest as examples.

Mike Lydon and others added 3 commits April 21, 2022 10:08
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.
@CodeLikeAGecko CodeLikeAGecko force-pushed the http-sink-errors-tolerance branch from 5c333f1 to e2c83ed Compare April 21, 2022 17:55
@CodeLikeAGecko CodeLikeAGecko requested a review from a team as a code owner April 21, 2022 17:55
@CodeLikeAGecko CodeLikeAGecko force-pushed the http-sink-errors-tolerance branch from e2c83ed to 984a7ba Compare April 21, 2022 19:19
@CodeLikeAGecko
Copy link
Contributor Author

@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!

Copy link
Contributor

@AnatolyPopov AnatolyPopov left a 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.

Changed JUnit assertions to AssertJ and fixed failing tests. Other
changes as requested.
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a 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.

build.gradle Outdated Show resolved Hide resolved
build.gradle 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.
@CodeLikeAGecko
Copy link
Contributor Author

@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.

Copy link
Contributor

@AnatolyPopov AnatolyPopov left a 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.

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
jlprat
jlprat previously requested changes May 13, 2022
Copy link
Contributor

@jlprat jlprat left a 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

build.gradle Show resolved Hide resolved
Updating versions to better align dependencies.
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a 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!

@AnatolyPopov AnatolyPopov dismissed jlprat’s stale review May 16, 2022 14:27

Comments addressed

@AnatolyPopov AnatolyPopov merged commit 6f8dcd6 into Aiven-Open:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants