-
Notifications
You must be signed in to change notification settings - Fork 241
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: add configuration options for Kafka consumer groups #436
feat: add configuration options for Kafka consumer groups #436
Conversation
f0e6aca
to
cf7024d
Compare
cf7024d
to
cbb405e
Compare
15058b9
to
8bfa829
Compare
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
d4cbb88
to
82148d8
Compare
82148d8
to
5e323c5
Compare
5e323c5
to
2f5965c
Compare
To make sure that this PR is not automatically closed - here’s a comment. |
6da3fd9
to
8f89eb8
Compare
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
To make sure that this PR is not automatically closed - here’s a comment...again... @darnaut Here's a reminder regarding this PR. :-) |
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
8f89eb8
to
b12a5c9
Compare
Hello, it is possible to push this PR, the fact of being able to configure prefixes allows me to solve problems on kafka which requires prefix namespace. @darnaut thank you |
b12a5c9
to
92f446a
Compare
Hi, we also have this issue so it would be really great to merge this |
Hi I also would need this functionality for my organization |
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.
LGTM
This PR adds configurable Kafka consumer groups to the Helm chart. With the exception of one consumer group, it is already possible today to configure the consumer groups via the extra environment variables (extraEnvs), unfortunately one of the environment variables (DATAHUB_UPGRADE_HISTORY_KAFKA_CONSUMER_GROUP_ID) is already present in the corresponding Deployment and can therefore not specified via the extra environment variables. Nonetheless similar to the configurable Kafka topics it would be great to have dedicated options for the consumer groups too.
Additionally this PR already uses three new environment variables added by another PR in the datahub-actions repository to also configure the consumer groups used by the default action pipelines. Of course the other PR has to be merged first and there must be a new release for the datahub-actions image, which must be used by the Helm chart so this will have any effect (this PR expects that this new release will be version v0.0.16, therefore this PR should not be merged until there is a new release!).
For the default values I have used the currently defined default values e.g. defined
here or here (I have simply searched for "KAFKA_CONSUMER_GROUP_ID" to get all consumer groups resp. the corresponding environment variables).
DATAHUB_UPGRADE_HISTORY_KAFKA_CONSUMER_GROUP_ID is a bit special in here: The three pods (datahub-gms, mae-consumer and mce-consumer) which are using this environment variable have different values defined, therefore there are three "subkeys" to configure each pod individually. Additionally the previous default value for each pod was defined using the release name of the chart, to be consistent with previous versions this logic is kept and the default value "next" to the Kafka listener (e.g., here) is not used.
This PR also solves #319.
Checklist