-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(inputs.kafka_consumer): Add regular expression support for topics #11831
Conversation
3e9696c
to
6fe3366
Compare
7e4ff5d
to
b139e0e
Compare
4af8375
to
50a80b4
Compare
cb2f57f
to
38fdfd2
Compare
The Avro changes have landed, and now there's a test suite that uses Docker to run Kafka containers, so I'm working on this again. |
85ec440
to
030e013
Compare
@athornton I agree with the comments of @Hipska. Can you please fix those and we are good to go. |
Co-authored-by: Thomas Casteleyn <[email protected]>
@athornton added two suggestions. Those should work and only require you to run |
Co-authored-by: Sven Rebhan <[email protected]>
Suggestions incorporated and "make docs" rerun. |
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.
Looks good to me. Thanks for the nice contribution @athornton!
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.
Hi,
What tells the consumer go routine to reload the topics?
There is a comment about signally the goroutine, but where does this happen?
We currently are not signaling to restart consumers on topic change. That's going to take some thought to implement without race conditions. I would prefer to tackle that as future work and close this one, which has been open eight months. |
Then I would need to ask you to remove the dynamic topic detection for the purpose of this PR. No reason to have it land when it does nothing. An update to the PR title and description would be good as well. |
Refresh interval removed. |
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.
Thanks!
Required for all PRs
This is a companion piece to #11816 and builds on top of it. It uses a connection to the Kafka broker to determine which topics are available, match them with a regular expression. This requires the set-metric-from-measurement in the Avro handler, which requires the schema registry, and therefore this is only useful in conjunction with the Avro parser.
In the Rubin Observatory use case, we connect to a Kafka broker with a large number of topics, which are added to dynamically, and we therefore require something like this. (For the time being, we will restart the broker to pick up new topics; dynamic detection is a planned future enhancement.)
The motivation is that this is essentially the behavior that lensesio/stream-reactor gives with its kafka-influx connector (which is what we have been using), except that that implementation only works with InfluxDBv1, while this will work with any Telegraf output. Since Rubin Observatory is trying to move to InfluxDBv2, we require something like this.
Added a configuration items to specify a list of regular expressions for topics to match; then I added an implementation for this item.