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 application name as default Kafka consumer group.id #15678

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Mar 12, 2021

Resolves #15632

@Ladicek Ladicek requested a review from cescoffier March 12, 2021 14:57
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 12, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@Ladicek Ladicek force-pushed the kafka-default-group-id branch from 86ccf73 to beecdd1 Compare March 12, 2021 14:57
@Ladicek Ladicek changed the title use application name as default Kafka consumer group.id Use application name as default Kafka consumer group.id Mar 12, 2021
Copy link
Member

@cescoffier cescoffier left a 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!

I just made a small comment.

docs/src/main/asciidoc/kafka.adoc Show resolved Hide resolved
@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 12, 2021

Draft to let CI run in my fork.

I was thinking how to add a test for this, but couldn't find anything. I don't think SmallRye Reactive Messaging lets me access the internals, through which I could verify if the group ID is really set. I did verify it manually with a simple example application though.

Also, #15632 says that a generated ID is only used in dev mode -- I couldn't find that anywhere in Quarkus code, and the SmallRye Reactive Messaging code generated a UUID unconditionally. So I think #15632 is just mistaken.

@geoand
Copy link
Contributor

geoand commented Mar 12, 2021

Interesting coincidence, I just opened #15680 :).

I'll rebase it once this is in.

@Ladicek Ladicek force-pushed the kafka-default-group-id branch from beecdd1 to f6db501 Compare March 12, 2021 15:09
@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 12, 2021

@geoand I'm fine with rebasing this on top of #15680 as well, so if you get to merging your first, please do :-)

@geoand
Copy link
Contributor

geoand commented Mar 12, 2021

No need, I'll wait :)

Base automatically changed from master to main March 12, 2021 15:55
@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 12, 2021

CI passed in my fork.

@Ladicek Ladicek marked this pull request as ready for review March 12, 2021 16:27
@geoand geoand merged commit 7f3769c into quarkusio:main Mar 12, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Mar 12, 2021
@Ladicek Ladicek deleted the kafka-default-group-id branch March 12, 2021 20:11
@wernert75
Copy link

In principle, I think that the change is good.
However, it now contradicts the doc:

"A unique string that identifies the consumer group the application belongs to. If not set, a unique, generated id is used
Type: string"

In a special case I need the previous behavior of a randomly generated group.id. How can I restore this behavior?

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 6, 2021

It shouldn't contradict the doc, as I updated the doc in the same commit :-)

If you want your group ID random, you can manually generate a random string and put it into an env var (KAFKA_GROUP_ID), for example.

@cescoffier
Copy link
Member

As I said on zulip, using a generated UUID needs to be a conscientious decision.

@wernert75
Copy link

wernert75 commented Apr 6, 2021

Thank you for your answer.

I agree with you. I think it's a good change. And using a generated UUID schould be a conscientious decision.

But the doc seems to be not updated on the website
https://quarkus.io/guides/kafka

And a mention in the Migration Guide would have saved me a lot of time. ;)

Thanks again. I will solve it via the configuration.

Perhaps you will also consider again whether an explicit configuration variable that activates the explicit use of a UUID is possible.

Basically, the issue is that several replicas of one and the same microservice get different Group.IDs, to broadcast a message.

It would be ideal if there could be a kind of "pool" for Group.ids. For example, MyApplication1 ... MyApplication5. If someone has an idea how to solve the problem of assigning exactly one of these GroupIds to each replica via a configuration, I would be very happy to hear about it.

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 6, 2021

Hmm I wonder how that website is updated, maybe I updated a wrong file? Humm...

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 6, 2021

Ah ah, now I know who to blame: 8a56a30

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 6, 2021

I'll submit a PR to re-add the new docs.

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 6, 2021

Submitted #16262.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use application name (if set) as Kafka group.id
4 participants