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

Use higher level kafka Writer api instead of low level Conn api #206

Merged
merged 1 commit into from
Mar 22, 2021

Conversation

zemek
Copy link
Contributor

@zemek zemek commented Mar 10, 2021

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area outputs

What this PR does / why we need it:
If no messages to the kafka broker has been sent within connections.max.idle.ms set on the broker (default 10 minutes) then the connection gets closed. The existing way of setting up the kafka connection does not re-open the connection and fails with error messages like:

10.1.2.13:42642->10.2.3.4:9092: write: broken pipe

I also think it's an anti-pattern to explicitly set the partition number to write to, this means we cannot balance the load across multiple brokers.


From the kafka-go readme:

To produce messages to Kafka, a program may use the low-level Conn API, but
the package also provides a higher level Writer type which is more appropriate
to use in most cases as it provides additional features:

  • Automatic retries and reconnections on errors.
  • Configurable distribution of messages across available partitions.
  • Synchronous or asynchronous writes of messages to Kafka.
  • Asynchronous cancellation using contexts.
  • Flushing of pending messages on close to support graceful shutdowns.

compared with: https://github.com/segmentio/kafka-go#connection-

The Conn type is the core of the kafka-go package. It wraps around a raw network connection to expose a low-level API to a Kafka server.


Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Signed-off-by: Joseph Zemek <joseph@robinhood.com>
@poiana poiana added the size/S label Mar 10, 2021
@Issif Issif requested a review from cpanato March 10, 2021 22:38
@Issif Issif added this to In progress in 2.x via automation Mar 10, 2021
@Issif Issif added this to the 2.22.0 milestone Mar 10, 2021
@Issif
Copy link
Member

Issif commented Mar 10, 2021

Thank you for this PR, it's really valuable. As it introduces a breaking change, we need to be very careful and clean in next Changelog.

@cpanato who's written this output will take a look for sure when he can. Thank you to both of you.

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm the code

/hold to get some confirmation if it can post messages to the kafka, I did not have time to do my own check

@poiana
Copy link

poiana commented Mar 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana
Copy link

poiana commented Mar 11, 2021

LGTM label has been added.

Git tree hash: 607747c8cc33f1ae48f27bea789b494c38a4971a

@cpanato
Copy link
Member

cpanato commented Mar 11, 2021

/retest

@leogr
Copy link
Member

leogr commented Mar 12, 2021

I'm trying to trigger the CI, so I'm closing and reopening
/close

@poiana poiana closed this Mar 12, 2021
@poiana
Copy link

poiana commented Mar 12, 2021

@leogr: Closed this PR.

In response to this:

I'm trying to trigger the CI, so I'm closing and reopening
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

2.x automation moved this from In progress to Done Mar 12, 2021
@leogr
Copy link
Member

leogr commented Mar 12, 2021

/reopen

@poiana
Copy link

poiana commented Mar 12, 2021

@leogr: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana poiana reopened this Mar 12, 2021
2.x automation moved this from Done to In progress Mar 12, 2021
@zemek
Copy link
Contributor Author

zemek commented Mar 22, 2021

@cpanato @Issif Is there anything else needed to merge this PR?

@Issif
Copy link
Member

Issif commented Mar 22, 2021

@zemek definitely not, I removed the hold label, hope it will unlock the merge

@poiana poiana merged commit 05ce32d into falcosecurity:master Mar 22, 2021
2.x automation moved this from In progress to Done Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
2.x
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants