Skip to content

NIFI-14630 Corrected usage of KafkaClientComponent interface#9993

Merged
exceptionfactory merged 2 commits intoapache:mainfrom
turcsanyip:NIFI-14630
Jun 17, 2025
Merged

NIFI-14630 Corrected usage of KafkaClientComponent interface#9993
exceptionfactory merged 2 commits intoapache:mainfrom
turcsanyip:NIFI-14630

Conversation

@turcsanyip
Copy link
Contributor

@turcsanyip turcsanyip commented Jun 5, 2025

The PR removes KafkaPublishComponent / KafkaClientComponent interface implementation from PublishKafka. The single property that was used from KafkaPublishComponent has been moved to the processor (as other publisher specific properties).
Kafka3ConnectionService now implements KafkaClientComponent in order for the properties be defined in a single location. The properties in the interface have been updated from the controller service (they were used from the service effectively, the interface just provided property names for the shared code).

Summary

NIFI-14630

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@mark-bathori mark-bathori left a comment

Choose a reason for hiding this comment

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

Thanks @turcsanyip for these fixes and corrections. The changes looks good.
LGTM +1

Comment on lines -38 to -40
.addValidator(StandardValidators.HOSTNAME_PORT_LIST_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
.defaultValue("localhost:9092")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not keeping this validator and the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for flagging this!

In general, I did not want to change the behavior of the properties in order to eliminate compatibility issues at all. So I took the property definitions from Kafka3ConnectionService because those were used effectively.

On the other hand, it makes sense to add HOSTNAME_PORT_LIST_VALIDATOR because the existing Bootstrap Servers property values must be following this format already. Otherwise the property is unusable. So this seems a minimal risk change and therefore I'm leaning toward adding the validator.

Regarding localhost:9092 default value: The "localhost" values are typically used during development only and they must be changed in real life use cases almost all the time. Personally, I don't prefer "developer defaults" exposed in prod apps (even if it is handy for devs). However, I see that we use "localhost" defaults in some places so if you think it is kind of a convention in NiFi, I can add it.

So I'm +1 for the validator and rather -1 for the default value. Please share what you think based on the background info provided above and I will change the code accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, fair enough, sounds good to me, thanks @turcsanyip

Copy link
Contributor

@exceptionfactory exceptionfactory Jun 12, 2025

Choose a reason for hiding this comment

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

I agree with removing the localhost:9092 default value, and instead having it null is best, thanks @turcsanyip and @pvillard31.

Copy link
Contributor

@pvillard31 pvillard31 left a comment

Choose a reason for hiding this comment

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

Thanks @turcsanyip - I had another look at the changes and I'm a +1
@exceptionfactory - feel free to merge if you're a +1 as well or I can do it

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks @turcsanyip, and thanks for the review @pvillard31, I concur with the changes. +1 merging

@exceptionfactory exceptionfactory merged commit e45d5d7 into apache:main Jun 17, 2025
7 checks passed
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.

4 participants