Skip to content

Conversation

chedim
Copy link

@chedim chedim commented Aug 1, 2025

Summary

This is part of NIFI-14731 that re-introduces GetCouchbaseKey and PutCouchbaseKey processors.
NIFI-14731

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 (had some issues with tests from un-modified by the PR files)
  • 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

@chedim chedim force-pushed the nifi-14731-getputcbkeyprocessors branch from 97d30cd to 3d5a5b8 Compare August 1, 2025 15:10
@chedim chedim marked this pull request as ready for review August 1, 2025 15:10
@chedim chedim force-pushed the nifi-14731-getputcbkeyprocessors branch from 3d5a5b8 to b352b7e Compare August 1, 2025 15:12
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 for providing a more narrowly-scoped implementation @chedim.

The initial automated builds are failing because the module version references now need to be updated to 2.6.0-SNAPSHOT.

On a very cursory review, I noticed several general issues to address:

  1. Additional Details documentation no longer supports HTML, it must be formatted using Markdown
  2. Having a different name() and displayName() for Property Descriptors is no longer recommend, instead, only name() should be used with a Title Case Name.
  3. The license file mentions an Bouncy Castle 1.5, which does not appear to be used
  4. Creating the list of Property Descriptors should use List.of() and relationships should use Set.of()
  5. Multline strings should be used instead of string concatenation for property descriptions
  6. The init() method should not be used on Processors
  7. Composition should be preferred over inheritance for most Processors, the multiple protected methods with empty default implementations make it difficult to follow
  8. Separating Property Descriptors into their own class makes it more difficult to follow in general, so this approach should be avoided

I realize a number of these things are likely inherited from the previous version of these components. However, now is the opportunity to address them properly.

Feel free to reopen the pull request when ready for review.

@chedim
Copy link
Author

chedim commented Aug 6, 2025

Thank you for the suggestions, I'll reopen the PR with these things fixed in the coming days.

@exceptionfactory
Copy link
Contributor

Thank you for the suggestions, I'll reopen the PR with these things fixed in the coming days.

Thanks @chedim, as mentioned on the other thread, the current PR #10031 should provide a good foundation going forward, so a new PR is not needed at this time.

@chedim
Copy link
Author

chedim commented Aug 7, 2025

@exceptionfactory Ouch, I've missed that comment. Do you still want me to update the other parts of the old CB plugin that I did not include in this PR?

@exceptionfactory
Copy link
Contributor

@exceptionfactory Ouch, I've missed that comment. Do you still want me to update the other parts of the old CB plugin that I did not include in this PR?

Thanks for asking. At this time, it would be better to wait and plan to build on the new implementation being worked in PR #10031. As that will provide new Controller Service interfaces, that will be a better foundation for introducing or reintroducing features.

@chedim
Copy link
Author

chedim commented Aug 8, 2025

Ok, I'll let my managers know that this needs to be paused. Thank you for your reviews and thank you for maintaining a great framework! You can always ping me or other folks at CB devx team if the plugin needs updating again :)

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