-
Notifications
You must be signed in to change notification settings - Fork 910
CRT S3 Client PutObject #2328
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
CRT S3 Client PutObject #2328
Conversation
@Override | ||
public boolean getRequestBytes(ByteBuffer bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've doing some testing against this updated interface, and CRT doesn't seem to treat the return value the way I'd expect, given this description.
I would expect the following to work
getRequestBytes(ByteBuffer bb) {
return false;
}
which results in getRequestBytes()
being called continuously, like this:
Content length is 4 bytes
1: getRequestBytes(buffer); // returns false, buffer contains 1 byte
2: getRequestBytes(buffer); // returns false, buffer contains 1 byte
3: getRequestBytes(buffer); // returns false, buffer contains 1 byte
4: getRequestBytes(buffer); // returns true, buffer contains 1 byte
but returning false results in the request completing instead; i.e. 2-4 never happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussing with CRT team about what we can do to support the use case above. For now, the implementation fills the buffer before returning
services/s3/src/it/java/software/amazon/awssdk/services/s3/CrtClientIntegrationTest.java
Show resolved
Hide resolved
...es/s3/src/main/java/software/amazon/awssdk/services/s3/internal/DefaultS3CrtAsyncClient.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/services/s3/internal/s3crt/RequestDataSupplierAdapter.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/services/s3/internal/s3crt/RequestDataSupplierAdapter.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/services/s3/internal/s3crt/RequestDataSupplierAdapter.java
Outdated
Show resolved
Hide resolved
services/s3/src/it/java/software/amazon/awssdk/services/s3/CrtClientIntegrationTest.java
Show resolved
Hide resolved
services/s3/src/it/java/software/amazon/awssdk/services/s3/CrtClientIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/services/s3/internal/s3crt/RequestDataSupplierAdapter.java
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/services/s3/internal/s3crt/RequestDataSupplierAdapter.java
Show resolved
Hide resolved
|
||
// TODO: not volatile since it's read and written only by CRT thread(s). Need to | ||
// ensure that CRT actually ensures consistency across their threads... | ||
private Subscriber<? super ByteBuffer> subscriber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only accessed by the same CRT thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, in my testing I saw two different threads calling getRequestData
. However, if the threads ares are synchronizing between each other, so that changes made by one thread are visible by the other thread then it should be fine. I'll check with CRT
Build failures due to dependency on unreleased version for CRT Java. Changes have been merged to mainline awslabs/aws-crt-java#298 |
…f66cf7077 Pull request: release <- staging/b05e44fe-3315-40f0-a450-b03f66cf7077
Description
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense