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

NIFI-11261 Add Primary Node State handling to GetAzureEventHub #7023

Closed
wants to merge 2 commits into from

Conversation

exceptionfactory
Copy link
Contributor

Summary

NIFI-11261 Adds Primary Node State Change handling to GetAzureEventHub to improve Processor behavior in a clustered deployment.

Although ConsumeAzureEventHub provides the recommend approach for receiving events, GetAzureEventHub can be configured to run on the cluster primary node only to reduce the potential for duplication.

Updates include checking for the Execution Node status and avoiding unnecessary Event Hub Consumer Client creation. The new handler method for Primary Node State changes closes the Consumer Client when a node has its primary status revoked, and creates a new Consumer Client when a node is elected primary.

Additional changes include upgrading the Qpid Proton J dependency from 0.34.0 to 0.34.1 and setting the Consumer Client Identifier to the Processor Identifier for improved connection tracking.

New unit test methods provide basic confirmation of Processor behavior when configured for primary node execution.

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 11
    • JDK 17

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

- Updated Qpid Proton J from 0.34.0 to 0.34.1
@turcsanyip turcsanyip self-requested a review March 9, 2023 10:12
Copy link
Contributor

@turcsanyip turcsanyip left a comment

Choose a reason for hiding this comment

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

@exceptionfactory Thanks for adding primary node handling to the processor!
I tested it on a cluster and works as expected. The code also looks good to me.
I have only a minor suggestion. Pls. see below.


// Set Azure Event Hub Client Identifier using Processor Identifier instead of default random UUID
final AmqpClientOptions clientOptions = new AmqpClientOptions();
clientOptions.setIdentifier(getIdentifier());
Copy link
Contributor

Choose a reason for hiding this comment

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

The processor's identifier is the same on all cluster nodes so all instances would use the same id.
When the processor is configured to run in "All nodes" mode, it can be confusing to have multiple clients with the same identifier. Also, in "Primary node" mode it would be useful to see which node the client belongs to.
So I suggest adding some node-specific tag in the identifier.

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 the suggestion, I agree it would be helpful to have the Node Identifier included. I pushed an update to prefix the Client Identifier with the Node Identifier when it is available in a clustered deployment.

Copy link
Contributor

@turcsanyip turcsanyip left a comment

Choose a reason for hiding this comment

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

Thanks @exceptionfactory !

+1 LGTM
Merging to main and 1.x

@asfgit asfgit closed this in dbc627e Mar 9, 2023
asfgit pushed a commit that referenced this pull request Mar 9, 2023
- Updated Qpid Proton J from 0.34.0 to 0.34.1

This closes #7023.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
r-vandenbos pushed a commit to r-vandenbos/nifi that referenced this pull request Apr 11, 2023
- Updated Qpid Proton J from 0.34.0 to 0.34.1

This closes apache#7023.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants