NIFI-12772 Expose REMOTE_POLL_BATCH_SIZE property for ListSFTP

[tombrisland]

Summary

NIFI-12772

Allows use of the REMOTE_POLL_BATCH_SIZE property in the ListSFTP processor. Previously this was supported but not exposed for configuration.

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 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

[tombrisland]

Need to do some manual testing

[EndzeitBegins]

Thank you for working on this @tombrisland.
I'm really looking forward to this improvement and hope we can backport it to 1.x as well.

I haven't had time to do any testing, but just glanced over the code changes and they look straightforward.

It seems that even though the SFTPTransfer supports the property already, the underlying sshj does not support a limit to ls directly but instead we rather stop adding results to our list. The listing is continued nonetheless.
I think supporting the property is still an improvement, as we at least can limit the amount of FlowFiles / records created this way.

However, I wonder if it makes sense to move the existing max result check

if (listing.size() >= maxResults) {
    return false;
}

to the top of the RemoteResourceFilter? The limit only ever gets relevant, when there is a large amount of files at the remote source. Even though we cannot abort the listing prematurely, at least not without modifications to sshj, this way the checks needed for all files after the limit is reached is kept to a minimum. This seems beneficial in situations where the listing takes a long time due files accumulated on the remote source.

What do you think about that?

[tombrisland]

Makes sense to me @EndzeitBegins, would also be great if we can get your early termination support in!

We may also need to have a think about which tracking strategies are supported when you select batch size. E.g. I don't think that time tracking will work with batch size since it relies on keeping track of the last timestamp processed?

[EndzeitBegins]

Thank you for the adjustments.
While I hope we can get the changes to land in sshj I wouldn't wait for them. After introducing the general support for REMOTE_POLL_BATCH_SIZE in ListSFTP, we still can integrate a new version of sshj that includes the improvements in a following PR.

That's a very sensible remark. I think it makes sense to limit the support for REMOTE_POLL_BATCH_SIZE to the strategies BY_ENTITIES and NO_TRACKING only. I'm not familiar with BY_TIME_WINDOW but I think combining the batch size with one of the other strategies might result in some files being skipped / never be picked up. On the other hand, ListFTP does not restrict the use to selected strategies, so there's that. 🤷🏻‍♂️

[exceptionfactory]

Thanks for working on this change @tombrisland, and thanks for the feedback thus far @EndzeitBegins.

Regarding making the property dependent, this should not be changed in FileTransfer because it also impacts ListFTP and GetFTP.

It does not seem necessary to make it dependent, because those other strategies could still work under some circumstances. The fact that it is not currently a dependent property for the FTP Processors is also worth noting, so at least for the initial change, it seems better to avoid making it a dependent property.

[joewitt]

I don't have a lot to add to the comments I see already. But the main gist is if the intent of the JIRA is now broader than the original title please change the JIRA. If the intent has not changed then please narrow the PR. So either this is about adding this option to ListSFTP or it is about others. Would be important to validate each case which expands the scope of the review.

Given we don't actually get to, it appears, change the behavior of the SFTP commands executed on the remote end (ie: once you start a listing in a large directory...much of the damage is already done in terms of time/waiting) then the benefit of this property being available is mostly to protect the NiFi side. Still useful just probably not the truly desired/most beneficial impact.

I'd avoid any changes for now that aren't central to the specific desired improvement.

[exceptionfactory]

Thanks for the update @tombrisland. I noticed one import that no longer seems necessary, and a had a question on one other relocation.

Also, if you could do this change as a regular push, instead of a force-push, that would be helpful for tracking. The initial PR should have a single commit, but after the review process is complete, then all the changes will be squashed into a single commit for merging into the main branch.

[EndzeitBegins]

Thanks for taking on the merge process @joewitt.
Could we backport these changes to the support/nifi-1.x branch as well?

[joewitt]

@EndzeitBegins It would need a separate JIRA/PR would be needed and I dont think the 'removeIf' language mechanism would work as NiFi 1.x is Java 8 or higher.

[EndzeitBegins]

Thank you for the fast response @joewitt and letting me know.

Hey @tombrisland, are you interested to work on a backport for 1.x or should I have a go at it?