Skip to content

Conversation

@ravinarayansingh
Copy link
Contributor

…st mark/reset support, add related tests.

Summary

NIFI-15025

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 ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

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

…st mark/reset support, add related tests.
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 proposing this improvement @ravinarayansingh.

On initial review, I'm concerned about introducing a custom InputStream implementation for this particular use case.

The RestLookupService is not designed for handling very large responses in general, although accommodating something larger seems reasonable. Instead of adding a custom InputStream with associated temporary file management, what do you think about simply using a larger buffer size for the BufferedInputStream? I agree that introducing a property is probably unnecessary, but increasing the buffer size to something like 1 MB or 8 MB seems like it would provide a reasonable size to cover more use cases, without introducing more complexity.

@ravinarayansingh
Copy link
Contributor Author

Thanks for proposing this improvement @ravinarayansingh.

On initial review, I'm concerned about introducing a custom InputStream implementation for this particular use case.

The RestLookupService is not designed for handling very large responses in general, although accommodating something larger seems reasonable. Instead of adding a custom InputStream with associated temporary file management, what do you think about simply using a larger buffer size for the BufferedInputStream? I agree that introducing a property is probably unnecessary, but increasing the buffer size to something like 1 MB or 8 MB seems like it would provide a reasonable size to cover more use cases, without introducing more complexity.

Hi @exceptionfactory
Agreed, an 8 MB buffer size sounds like the right balance. I’ll go ahead and make the change accordingly

@exceptionfactory
Copy link
Contributor

Hi @exceptionfactory Agreed, an 8 MB buffer size sounds like the right balance. I’ll go ahead and make the change accordingly

Thanks @ravinarayansingh, that sounds good.

…stLookupService`, update tests to validate behavior with buffer limits
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 adjusting the approach and adding the test @ravinarayansingh, this looks good! +1 merging

@exceptionfactory exceptionfactory merged commit 909c06d into apache:main Oct 2, 2025
7 checks passed
@exceptionfactory exceptionfactory added the hacktoberfest-accepted Hacktoberfest Accepted label Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Hacktoberfest Accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants