Skip to content

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Jun 21, 2023

No description provided.

@findinpath
Copy link
Contributor

@trinodb/maintainers pls run the PR with secrets

Copy link
Member

Choose a reason for hiding this comment

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

This was here for a reason

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Now when you changed fromByteBufferUnsafe it should be fine.
@linzebing PTAL.

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Restore ByteBufferAsyncRequestBody.

BTW: Even if we wanted to drop it - it should be separate commit - not SDK version update

@wendigo
Copy link
Contributor Author

wendigo commented Jun 21, 2023

@wendigo wendigo force-pushed the serafin/deps-upgrades branch from 485901d to 4776e1d Compare June 21, 2023 13:26
@wendigo
Copy link
Contributor Author

wendigo commented Jun 21, 2023

@losipiuk extracted to separate commit

@wendigo
Copy link
Contributor Author

wendigo commented Jun 21, 2023

@losipiuk I think this part is relevant:

The static methods in AsyncRequestBody do some defensive copying and transforming of types that hamper performance and utilise more memory than needed if the user has already ensured either that concurrent modification won't happen or that it doesn't matter, this pr addresses that with new AsyncRequestBody constructors.

This also fixes an unnecessary copy being made in AsyncRequestBody#fromByteBuffer (https://github.com/aws/aws-sdk-java-v2/issues/1735) and AsyncRequestBody#fromString as a side effect.

@wendigo wendigo requested a review from losipiuk June 21, 2023 13:28
@wendigo
Copy link
Contributor Author

wendigo commented Jun 21, 2023

There is also pending PR which seems relevant here: aws/aws-sdk-java-v2#4096:

This change is effectively running in my own production code where I upload files via [MappedByteBuffer](https://docs.oracle.com/javase/8/docs/api/java/nio/MappedByteBuffer.html), when performance testing I was able to achieve a 25% performance improvement over TransferManager uploading a 1GB file.

@wendigo wendigo force-pushed the serafin/deps-upgrades branch from 4776e1d to 87e9678 Compare June 21, 2023 13:35
@losipiuk
Copy link
Member

/test-with-secrets 7d308de

@losipiuk losipiuk dismissed their stale review June 21, 2023 13:44

issue removed

@losipiuk
Copy link
Member

/test-with-secrets sha=7d308deed56c6cc619d16b1b7a18a7713972e586

@electrum
Copy link
Member

Looks like we need to squash the commits, since the internal class ByteArrayAsyncRequestBody is removed in the new version, and thus the first commit won't compile on its own.

@electrum electrum merged commit 85befd9 into trinodb:master Jun 21, 2023
@github-actions github-actions bot added this to the 420 milestone Jun 22, 2023
@wendigo wendigo deleted the serafin/deps-upgrades branch June 22, 2023 04:07
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.

5 participants