[NIFI-10792] Fixed bug to allow for processing files larger than 10MB…

[dan-s1]

…. Also refactored to reduce the cognitive complexity in the onTrigger method.

Summary

NIFI-10792

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

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

[dan-s1]

@exceptionfactory I did not end up including the unit test I had as it was a unit which tested with a 20MB file. I would have thought there should be a unit test to exercise the change I made. Please advise.

[mh013370]

@exceptionfactory I did not end up including the unit test I had as it was a unit which tested with a 20MB file. I would have thought there should be a unit test to exercise the change I made. Please advise.

Anecdotally, the NiFi mock framework will read the entire FF contents into memory (I discovered this in #6369) so I think you're correct in not including unit tests testing larger FFs.

Here's my except from that PR

I attempted to unit test this, but due to how the NiFi mock framework works, every flowfile is read into memory in full even if that FlowFile is backed by a NullInputStream. I couldn't find a nice alternative to verify with a unit test.

[exceptionfactory]

Thanks for the comment @mh013370, this issue is similar to the size limits for Tar files resolved in #6369.

@dan-s1, Although unit tests are the preferred way to confirm expected behavior, unit tests are not optimal for testing these kinds of scenarios. For this particular situation, confirming existing functionality is good, and runtime verification is better than introducing large files or long-running tests into the repository.

[dan-s1]

@exceptionfactory When you get a chance can you please look over this PR? I was hoping I could reuse the Excel part of this code for NIFI-11167. I noticed a drawback with fastexcel-reader that it

discards styles, graphs, and many other stuff

therefore I would like to use excel-streaming-reader as I did in this PR as I believes it preserves these things or at least a subset of them.

[exceptionfactory]

Thanks @dan-s1, I plan to take a closer look at this pull request soon.

The excel-streaming-reader seems acceptable, although discarding styles, graphs, and other elements does not necessarily sound like a problem because that would not necessarily translate to record-oriented data for other services.

[dan-s1]

@exceptionfactory Thanks! That is a good point regarding the formatting. Though I would think you would want to have an option to preserve formatting for use with an Excel record writer.

[exceptionfactory]

Good point @dan-s1, I'm not sure if the formatting information could be carried through to an Excel Writer, but it would be useful if it can be supported.

[emiliosetiadarma]

Hey, I was able to verify that I was able to convert an excel spreadsheet > 10 MB successfully into CSV, I just have a few questions

[dan-s1]

@emiliosetiadarma @exceptionfactory I believe I addressed the issues brought up. Is there anything else needed?

[exceptionfactory]

Thanks for your work on this improvement @dan-s1, and thanks for the testing @emiliosetiadarma.

The code changes for streaming look good, existing unit tests pass and I was able to convert a 20 MB XLSX file at runtime with these changes.

I pushed a commit that adds the log4j-to-slf4j dependency, which was something missing after recent Apache POI upgrades, supporting log routing from Log4j 2 to the NiFi standard SLF4J. I also removed the explicit dependency on graphics2d, which has a holdover from earlier Apache POI versions and no longer used. Lastly, I reverted the reformatting of the properties and relationships, since it is easier to maintain the list of one per line versus wrapping lines.

With these changes, planning to merge following a successful automated build.

[exceptionfactory]

Automated builds look good. +1 merging