Skip to content

Conversation

@vishalya
Copy link
Member

@vishalya vishalya commented Jul 9, 2025

Description

  • The return values of the function isFinished should be true only no bytes are available in the input stream to read
  • isReady should always return true, since the inputstream should be ready all the time and never be false.
  • Added methods to so that multiple bytes can be read for better performance.

Additional context and related issues

@Chaho12 has idea on the issue with this code. @willmostly is the original author.

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Jul 9, 2025
@Chaho12
Copy link
Member

Chaho12 commented Jul 9, 2025

Typo in commit message. MultiReadHttpServletRequest misses t there.
Btw how about "Fix ServletInputStream behavior for MultiReadHttpServletRequest and add read override" or you can remove MultiReadHttpServletRequest.

public int read(byte[] b, int off, int len)
throws IOException
{
return byteArrayInputStream.read(b, off, len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to make this null-safe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byteArrayInputStream is locally allocated, if that's what your concern is. It should not be null.

@mosabua
Copy link
Member

mosabua commented Jul 18, 2025

Is this good to to @vishalya @Chaho12 @oneonestar ?

@Chaho12
Copy link
Member

Chaho12 commented Jul 18, 2025

Seems ok. But don't forget to fix the commit message :)

Copy link
Member

@andythsu andythsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for the commit msg

@vishalya vishalya changed the title Fix/enhance the MultiReadHttpServletReques stream Fix/enhance the MultiReadHttpServletRequest stream Jul 22, 2025
@vishalya vishalya changed the title Fix/enhance the MultiReadHttpServletRequest stream Enhance the MultiReadHttpServletRequest stream Jul 23, 2025
Copy link
Member

@Chaho12 Chaho12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Chaho12
Copy link
Member

Chaho12 commented Aug 18, 2025

Build failed because of this issue #755

@ebyhr
Copy link
Member

ebyhr commented Aug 19, 2025

Enhance the MultiReadHttpServletRequest stream

I don't think developers can understand changes from this commit title. Please add the commit body explaining what was enhanced.

@vishalya
Copy link
Member Author

Updated the PR description.

@ebyhr
Copy link
Member

ebyhr commented Aug 20, 2025

Thanks, please update the commit body too.

@mosabua
Copy link
Member

mosabua commented Aug 22, 2025

Lets finish this off @vishalya .. add the info from the description to the commit message as discussed in team sync and then you can go ahead with merge.

@vishalya vishalya merged commit 44a332e into trinodb:main Aug 25, 2025
2 checks passed
@github-actions github-actions bot added this to the 16 milestone Aug 25, 2025
@ebyhr
Copy link
Member

ebyhr commented Aug 26, 2025

@vishalya Not sure why you ignored the above comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants