-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15286 Verify nodes connect-ability in Kafka3ConnectionService #10596
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
Conversation
If not-authorized to describe the cluster, verify connect-ability to bootstrap servers.
exceptionfactory
left a comment
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.
Thanks for providing this improvement @dariuszseweryn. The general approach looks good. I noted some initial recommendations, and plan to take a closer look at some of the implementation details.
...hared/src/test/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifierTest.java
Outdated
Show resolved
Hide resolved
...ervice-shared/src/test/java/org/apache/nifi/kafka/service/Kafka3ConnectionServiceBaseIT.java
Outdated
Show resolved
Hide resolved
...hared/src/test/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifierTest.java
Outdated
Show resolved
Hide resolved
...ce-shared/src/main/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifier.java
Outdated
Show resolved
Hide resolved
...ce-shared/src/main/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifier.java
Outdated
Show resolved
Hide resolved
...ce-shared/src/main/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifier.java
Outdated
Show resolved
Hide resolved
...ce-shared/src/main/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifier.java
Outdated
Show resolved
Hide resolved
...i-kafka-service-shared/src/main/java/org/apache/nifi/kafka/service/KafkaClusterVerifier.java
Show resolved
Hide resolved
...hared/src/test/java/org/apache/nifi/kafka/service/verification/KafkaClusterVerifierTest.java
Outdated
Show resolved
Hide resolved
exceptionfactory
left a comment
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.
Thanks again for putting this together @dariuszseweryn.
On closer review, I noticed a few issues, particularly in relation to the order of operations. This initial approach verifies node connectivity after listing topics, which already implies some level of connectivity. I also noticed the introduction of several intermediate record context classes which did not seem necessary after some restructuring.
To help move things forward, I put together a new pull request #10689 and listed you as a co-author on the commit.
|
Closing this one as #10689 will be merged shortly. |
If not-authorized to describe the cluster, verify connect-ability to bootstrap servers.
Summary
NIFI-15286
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-15286NIFI-15286Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation