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

fix(kafka sink): retry messages that result in kafka policy violations #22041

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PriceHiller
Copy link
Contributor

Summary

Copied from the body of the commit:

Problem: Some messages were getting dropped by Vector due to Kafka throwing PolicyViolation errors. These should be retried as a policy can be as simple as a more aggressive rate limit.


Solution: Retry any messages that had the RDKafkaErrorCode::PolicyViolation error.


Note: A dynamic back off may be better, as there may be a rate limit out there that still needs more than 100ms to back off on requests.


See the original issue at #22026

Closes #22026

Ensures Kafka retries messages that error out with RDKafkaErrorCode::PolicyViolation.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

  • cargo test --no-default-features --features "sinks-console,sinks-kafka,kafka-integration-tests" -- --skip 'sinks::kafka::tests::integration_test' 'sinks::kafka'
  • make test-integration-kafka
    • NOTE: I am facing a few errors when running the integration tests both with and without this patch. Are they currently broken by any chance?
      Has to do with sources::kafka::integration_test::drains_acknowledgements_during_rebalance_default_assignments

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

@PriceHiller PriceHiller requested a review from a team as a code owner December 16, 2024 20:00
@github-actions github-actions bot added the domain: sinks Anything related to the Vector's sinks label Dec 16, 2024
@PriceHiller PriceHiller changed the title fix(kafka sink): retry messages that reuslt in kafka policy violations fix(kafka sink): retry messages that result in kafka policy violations Dec 16, 2024
@PriceHiller PriceHiller force-pushed the fix-kafka-sink-dropping-msgs branch from 3fb7bb2 to 3eaeee3 Compare December 16, 2024 20:01
Problem: Some messages were getting dropped by Vector due to Kafka
throwing `PolicyViolation` errors. These should be retried as a policy
can be as simple as a more aggressive rate limit.

----------

Solution: Retry any messages that had the `RDKafkaErrorCode::PolicyViolation` error.

----------

Note: A dynamic back off may be better, as there may be a rate limit out
there that still needs more than 100ms to back off on requests.

----------

See the original issue at vectordotdev#22026

Closes vectordotdev#22026
@PriceHiller PriceHiller force-pushed the fix-kafka-sink-dropping-msgs branch 2 times, most recently from 296551f to 3bfca14 Compare December 16, 2024 20:12
@PriceHiller
Copy link
Contributor Author

Realized I did not need to include that changelog entry for this PR.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Actually could we add a changelog entry here describing the fix for users? See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md

@PriceHiller PriceHiller force-pushed the fix-kafka-sink-dropping-msgs branch from 3bfca14 to 296551f Compare December 16, 2024 20:55
@PriceHiller
Copy link
Contributor Author

Actually could we add a changelog entry here describing the fix for users? See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md

Reincluded the changelog entry.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks @PriceHiller !

@jszwedko jszwedko enabled auto-merge December 16, 2024 21:04
@jszwedko jszwedko added this pull request to the merge queue Dec 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: sinks Anything related to the Vector's sinks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages getting dropped unintentionally on kafka sink
2 participants