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.

Sign up for GitHub

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

Jump to bottom

KAFKA-15145: Don't re-process records filtered out by SMTs on Kafka client retriable exceptions in AbstractWorkerSourceTask #13955

Merged
merged 2 commits into from
Jul 10, 2023
Merged

KAFKA-15145: Don't re-process records filtered out by SMTs on Kafka client retriable exceptions in AbstractWorkerSourceTask #13955

merged 2 commits into from
Jul 10, 2023

Conversation

yashmayya
Copy link
Contributor

@yashmayya yashmayya commented Jul 4, 2023 •
edited
Loading

From https://issues.apache.org/jira/browse/KAFKA-15145:

If a RetriableException is thrown from an admin client or producer client operation in AbstractWorkerSourceTask::sendRecords, the send operation is retried for the remaining records in the batch. There is a bug in the logic for computing the remaining records in a batch which causes records that are filtered out by the task's transformation chain to be re-processed. This will also result in the SourceTask::commitRecord method being called twice for the same record, which can cause certain types of source connectors to fail. This bug seems to exist since when SMTs were first introduced in 0.10.2

Committer Checklist (excluded from commit message)

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

@yashmayya
KAFKA-15145: Don't re-process records filtered out by SMTs on Kafka c…
ec7fb9a 
…lient retriable exceptions in AbstractWorkerSourceTask
C0urante
C0urante reviewed Jul 6, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yash, great catch! Left one thought on testing and a possible behavior change below; LMKWYT.

...ect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTaskTest.java Outdated Show resolved Hide resolved
vamossagar12
vamossagar12 suggested changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@vamossagar12 vamossagar12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Added some minor comments/suggestions.

...ect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTaskTest.java Show resolved Hide resolved
vamossagar12
vamossagar12 approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@vamossagar12 vamossagar12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

C0urante
C0urante approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

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

Let me know how you feel about the re-transforming-on-retry discussion. Happy to hold off on merging if you'd like to tweak this more, but otherwise, I'm comfortable with these changes and am ready to merge+backport.

@yashmayya
Copy link
Contributor Author

Thanks Chris, I agree with the point you brought up regarding potential poison pill messages. This PR can be merged.

@C0urante C0urante merged commit 9ee28d1 into apache:trunk Jul 10, 2023
1 check failed
C0urante pushed a commit that referenced this pull request Jul 10, 2023
@yashmayya @C0urante
KAFKA-15145: Don't re-process records filtered out by SMTs on Kafka c…
031e300 
…lient retriable exceptions in AbstractWorkerSourceTask (#13955)

Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Chris Egerton <chrise@aiven.io>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
@yashmayya
KAFKA-15145: Don't re-process records filtered out by SMTs on Kafka c…
3fac756 
…lient retriable exceptions in AbstractWorkerSourceTask (apache#13955)

Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Chris Egerton <chrise@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@C0urante C0urante C0urante approved these changes

@vamossagar12 vamossagar12 vamossagar12 approved these changes

Assignees
No one assigned
Labels
connect
Projects
None yet
Milestone
No milestone
3 participants
@yashmayya @C0urante @vamossagar12