Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(java): allow streaming byte requests #4765

Merged
merged 21 commits into from
Sep 27, 2024

Conversation

dcb6
Copy link
Contributor

@dcb6 dcb6 commented Sep 26, 2024

No description provided.

@@ -58,4 +61,9 @@ public void upload(byte[] request, RequestOptions requestOptions) {
throw new SeedBytesException("Network error executing HTTP request", e);
}
}

// Overload for backward compatibility
public void upload(byte[] request, RequestOptions requestOptions) {
Copy link
Contributor Author

@dcb6 dcb6 Sep 26, 2024

Choose a reason for hiding this comment

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

thoughts on moving the direct bytes request to this?

Claude gave me these downsides but to me seems worth the cleanliness / reuse

Potential downsides of the upload overload:

Memory Usage:

The original implementation used the byte array directly.
The new overload wraps the byte array in a ByteArrayInputStream, which is then wrapped in a FileStream.
While ByteArrayInputStream is generally efficient, it does create an additional object in memory.


Performance Overhead:

There's a slight performance cost in creating the additional objects (ByteArrayInputStream and FileStream).
The streaming approach might introduce a small amount of overhead compared to sending the byte array directly, especially for small files."

Copy link
Member

Choose a reason for hiding this comment

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

great idea

/**
* Represents a file stream with associated metadata for file uploads.
*/
public class FileStream {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't actually need this for bytes requests, going to delete unless anyone votes I keep it around

Copy link
Member

Choose a reason for hiding this comment

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

awesome, may be useful to keep around to make it better for multipart (just as a uncopied utility)

@dcb6 dcb6 changed the title feat(java): allow streaming file uploads feat(java): allow streaming byte requests Sep 27, 2024
@dcb6 dcb6 marked this pull request as ready for review September 27, 2024 15:35
@dcb6 dcb6 requested a review from dsinghvi as a code owner September 27, 2024 15:35
- changelogEntry:
- summary: |
Bump Jackson version to latest (2.17.2)
type: chore
createdAt: '2024-09-26'
irVersion: 46
version: 1.2.0
version: 2.2.0
Copy link
Contributor Author

@dcb6 dcb6 Sep 27, 2024

Choose a reason for hiding this comment

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

this was a blunder 😅

thankfully, we never previously released a 1.2.0 and niels won't be using that until next week
I deleted this erroneous 1.2.0 tag from docker hub

Copy link

github-actions bot commented Sep 27, 2024

@dcb6 dcb6 merged commit b6bda91 into main Sep 27, 2024
53 checks passed
@dcb6 dcb6 deleted the burke/fer-2593-assemblyai-support-streams-for-file-uploads branch September 27, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants