[GitHub] [nifi] hawko2600 commented on a diff in pull request #6294: NIFI-2827 - Add support for ZSTD compression format

[GitBox <gi...@apache.org>]

hawko2600 commented on code in PR #6294:
URL: https://github.com/apache/nifi/pull/6294#discussion_r962205842


##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/CompressContent.java:
##########
@@ -328,6 +335,11 @@ public void process(final InputStream rawIn, final OutputStream rawOut) throws I
                                     mimeTypeRef.set("application/x-lz4-framed");
                                     compressionOut = new CompressorStreamFactory().createCompressorOutputStream(compressionFormat.toLowerCase(), bufferedOut);
                                     break;
+                                case COMPRESSION_FORMAT_ZSTD:
+                                    final int zstdcompressionLevel = context.getProperty(COMPRESSION_LEVEL).asInteger() * 2;

Review Comment:
   I used the same format variable name as per the similar variable for other formats, unsure of the issue. Literally copied the line, cut the other format label out, replaced with zstd.
   
   I noted this on Jira; zstd has 22 compression levels, though they advise not to use more than 20. It effectively changes block size similar to gzip - there's just more refined tweaking available. Sadly at this point in the nifi code there's no easy way to support all 22 levels as nifi is setup for the 9 levels of other formats. I know it's not ideal but multiplying the value by 2 spreads the 9 levels of nifi across 18 of the 20 preferred zstd levels effectively the same as other formats, whilst retaining the zstd function of level 0 meaning "I don't know, let the algorithm figure it out" as 2*0=0.
   
   I was adding support for this one format, not trying to refactor how compression is supported generally. Over not having it at all, I'll take the simple approach and a more general "now that we have it, how do we effect all 22 levels" can be addressed in a separate issue.
   
   On the phone version of GitHub I can't seem to reply to the first comment, but the NOTICE files already have the necessary info as zstd was included for years via apache commons and so the license was already there. The only thing missing was the supporting code to enable the format.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org