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

GH-3001: default clientIds with application name #3048

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

notizklotz
Copy link
Contributor

Fixes: #GH-3001

Use Spring Boot's spring.application.name property as part of the default clientIds for Consumers, Producers and AdminClients. Helps with identifying problematic clients at server side.

  • Only use as a fallback if clientId wasn't specified explicitly
  • Do not use for Consumers with a specified groupId because KafkaConsumer will use the groupId as clientId which already is an identifiable default

@sobychacko
Copy link
Contributor

@notizklotz I am Starting to review the PR. There are some build failures due to checkstyle issues. See here.
Also, you need to add some docs about these changes.

@sobychacko sobychacko added this to the 3.2.0-M2 milestone Feb 28, 2024
@notizklotz
Copy link
Contributor Author

@notizklotz I am Starting to review the PR. There are some build failures due to checkstyle issues. See here. Also, you need to add some docs about these changes.

@sobychacko I fixed the checkstyle errors and added some docs to the "What’s new?" section

@sobychacko
Copy link
Contributor

sobychacko commented Mar 1, 2024

@notizklotz Thanks. The problem with adding this in the whats-new doc is that this will be replaced for the next version (3.3.0). This deserves a separate small section inside the reference docs. Maybe add a new section for configuring client.id? Then a short sentence in the whats-new section. One other thing - can you add your name as the @author in all the classes you modified and update any copyright years?

Fixes: #spring-projectsGH-3001

Use Spring Boot's `spring.application.name` property as part of the default clientIds for Consumers, Producers and AdminClients. Helps with identifying problematic clients at server side.

* Only use as a fallback if clientId wasn't specified explicitly
* Do not use for Consumers with a specified groupId because KafkaConsumer will use the groupId as clientId which already is an identifiable default
@notizklotz
Copy link
Contributor Author

@sobychacko Added section to reference docs, added author tag, checked copyright years, squashed commits and rebased to upstream main

@sobychacko
Copy link
Contributor

Thanks @notizklotz for the updates. One last comment - What do you think about extracting spring.application.name text literal into a common property since it is repeated in a handful of places?

@notizklotz
Copy link
Contributor Author

Thanks @notizklotz for the updates. One last comment - What do you think about extracting spring.application.name text literal into a common property since it is repeated in a handful of places?

@sobychacko I considered that but decided against it, because I couldn't find an existing class where I think it would be a good fit. I didn't want to introduce a new class like "Constants" just for this literal. I also saw that the "client-id" literal is also copy/pasted across various classes and I thought to stick with the existing style.

@sobychacko
Copy link
Contributor

Fair enough. We can always refactor that later if need be.

@sobychacko sobychacko removed this from the 3.2.0-M2 milestone Mar 11, 2024
@sobychacko sobychacko merged commit ab5f0a1 into spring-projects:main Mar 11, 2024
3 checks passed
@notizklotz notizklotz deleted the GH-3001 branch March 12, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants