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-11466: Add ModifyCompression processor #7180

Closed
wants to merge 6 commits into from

Conversation

mattyb149
Copy link
Contributor

Summary

NIFI-11466 This PR adds a ModifyCompression processor which combines the functionality of CompressContent with the ability to decompress the input then recompress the output, avoiding any disk usage between multiple CompressContent processors (decompress -> recompress) and uses sensible in-memory buffers to avoid out-of-memory errors.

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

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 introducing this new Processor @mattyb149! It looks like a helpful addition and potential replacement for CompressContent. With more recent framework updates, there are a number of improvement opportunities that could be made in this implementation.

One other high level suggestion would be to move this processor out of nifi-standard-nar and into a new nifi-compress-bundle. This Processor has a lot of external dependencies that are specific to compression, so this is a good opportunity to move them out of the standard NAR and into a specialized bundle.

@mattyb149 mattyb149 force-pushed the NIFI-11466 branch 2 times, most recently from da7e47f to 08099ff Compare April 24, 2023 02:57
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 making the updates @mattyb149. There are still a few unaddressed comments from the original round of feedback, and I noted several additional recommendations.

It looks like the unit tests are failing in the latest commit. StreamUtils.copy() does not call OutputStream.flush(), but the callback should call OutputStream.close(), not sure if that is related to the unit test problem.

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 the latest round of updates @mattyb149. I marked most of my comments as resolved, but there are several still unaddressed, including a few related to the test class. GitHub collapses larger sets of comments, so please make sure to expanded the list to make sure they are addressed.

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 the updates @mattyb149! I went through and did some runtime testing, and also took one more pass through the code. There were some opportunities to make more use the enum values for comparison, instead of comparing the string values, so I made some adjustments and pushed an update. The changes should be ready to go following a clean build.

@mattyb149
Copy link
Contributor Author

+1 LGTM, thanks for the assist! Do you mind merging this after adding yourself as a co-author? Want to make sure you get credit even after the commits are squashed.

@exceptionfactory
Copy link
Contributor

Thanks for the confirmation on the changes @mattyb149, will plan on merging with the Co-authored-by line soon.

mattyb149 and others added 6 commits April 29, 2023 21:01
- Renamed CompressionInfo to CompressionStrategy
- Moved Filename Strategy options to FilenameStrategy
- Adjusted conditionals to use CompressionStrategy instead of string values
- Removed unused MIME Type attribute check for Output Compression Strategy
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.

Rebased to resolve minor conflict in nifi-assembly/pom.xml. +1 merging

exceptionfactory pushed a commit that referenced this pull request Apr 30, 2023
- Added nifi-compress-bundle with nifi-compress-nar

Backported #7180 and set version to 1.22.0-SNAPSHOT

Co-authored-by: David Handermann <exceptionfactory@apache.org>
Signed-off-by: David Handermann <exceptionfactory@apache.org>

(cherry picked from commit 0e93dfa)
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