Skip to content

KafkaTestUtils unified consumerPros call #3938

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

Merged
merged 1 commit into from
Jun 4, 2025

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Jun 1, 2025

Currently, the KafkaTestUtils#consumerProps overloaded methods I think have a couple of problems:

  1. Their arguments order is not aligned with each other:
(String group, String autoCommit, EmbeddedKafkaBroker embeddedKafka)
(String brokers, String group)
(String brokers, String group, String autoCommit)

The group goes either the first one, or the second one, the same goes for autocommit setting

  1. The enable.auto.commit is a boolean according to doc, so I think accepting a String is not type safe, since the value. Making the API more typesafe I think is generally a good idea.

@mipo256 mipo256 force-pushed the type-safe-auto-commit branch from 1797755 to 4104734 Compare June 1, 2025 20:29
@mipo256 mipo256 changed the title Type safe auto commit KafkaTestUtils unified consumerPros call Jun 1, 2025
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, add your name to the @author list of the affected class.
Thanks

@mipo256 mipo256 force-pushed the type-safe-auto-commit branch 2 times, most recently from 7bf7ded to 36400ec Compare June 3, 2025 12:29
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, build the project locally and look into logs.
There are a bunch of warnings like this:

/home/runner/work/spring-kafka/spring-kafka/spring-kafka-test/src/test/java/org/springframework/kafka/test/utils/KafkaTestUtilsTests.java:58: warning: [removal] consumerProps(String,String,EmbeddedKafkaBroker) in KafkaTestUtils has been deprecated and marked for removal
		Map<String, Object> consumerProps = KafkaTestUtils.consumerProps("ktuTests1", "false", broker);

I expect that new API would be used whenever it is still rely on just deprecated one.
Thanks

@mipo256
Copy link
Contributor Author

mipo256 commented Jun 3, 2025

I agree, I'll migrate the usages fo the deprecated API. The .editorconfig commit really seems to be a bit off-topic, I'll roll it back.

@sobychacko
Copy link
Contributor

I wonder whether we can make these changes in 3.3.x so that we can remove the deprecated methods from 4.0.0 altogether?

@mipo256 mipo256 force-pushed the type-safe-auto-commit branch from 36400ec to ea4f7b9 Compare June 4, 2025 08:08
@mipo256 mipo256 requested a review from artembilan June 4, 2025 08:08
@mipo256
Copy link
Contributor Author

mipo256 commented Jun 4, 2025

That is a good point, @sobychacko, I also thought about it. I think this is open to discussion. The question is do we want to provide the extended grace period, or we do not. The KafkaTestUtil is quite heavily used, and spring-kafka 4.0 is going to be released relatively soon. So, I'll leave the resolution of this to you guys 😄.

P.S: @artembilan I have cleaned-up all the usages of the deprecated API

@mipo256 mipo256 force-pushed the type-safe-auto-commit branch from ea4f7b9 to 9003989 Compare June 4, 2025 09:06
@artembilan artembilan added this to the 4.0.0-M3 milestone Jun 4, 2025
@artembilan artembilan merged commit 05d6494 into spring-projects:main Jun 4, 2025
3 checks passed
@artembilan
Copy link
Member

Thank you for contribution, @mipo256 !

Please, consider to fix your Git client to add your real Mikhail Polivakha name to the signed-off-by sentence in the commit message.

@mipo256
Copy link
Contributor Author

mipo256 commented Jun 4, 2025

Sure @artembilan. Thank you!

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.

3 participants