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.

Sign up for GitHub

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

Jump to bottom

Adding a helper that exposes an async string request provider #183

Merged
merged 1 commit into from
Sep 27, 2017
Merged

Adding a helper that exposes an async string request provider #183

merged 1 commit into from
Sep 27, 2017

Conversation

kiiadi
Copy link
Contributor

@kiiadi kiiadi commented Sep 26, 2017

Can be used (for example) for uploading to s3:

S3AsyncClient s3 = ...;
s3.putObject(PutObjectRequest.builder().bucket("bucket-name").key("some-key.txt").build(),
             AsyncRequestProvider.fromString("hello world!!!"));

rleibman
rleibman approved these changes Sep 26, 2017
View reviewed changes
Copy link

@rleibman rleibman left a comment

Choose a reason for hiding this comment

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

Beautiful, thanks!

shorea
shorea reviewed Sep 26, 2017
View reviewed changes
Copy link
Contributor

@shorea shorea left a comment

Choose a reason for hiding this comment

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

Nice!

core/src/main/java/software/amazon/awssdk/async/SingleByteArrayAsyncRequestProvider.java Outdated
private final byte[] bytes;

private SingleByteArrayAsyncRequestProvider(DefaultBuilder builder) {
this.bytes = builder.bytes.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does clone do for a byte array? Is it a deep copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's just a reference copy, but since byte is primitive it shouldn't matter.

core/src/main/java/software/amazon/awssdk/async/SingleByteArrayAsyncRequestProvider.java Outdated
Builder bytes(byte[] bytes);
}

public static class DefaultBuilder implements Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

For internal stuff I think we can skip the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following the pattern we used for https://github.com/aws/aws-sdk-java-v2/blob/master/core/src/main/java/software/amazon/awssdk/async/FileAsyncRequestProvider.java

but I'm happy to skip the builder - we can always add it later if we ever want to make this public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I made that public since there were some options worth exposing. If we don't expose the class I'd stick with a constructor for now.

core/src/test/java/software/amazon/awssdk/async/AsyncRequestProviderTest.java Outdated
}


private abstract class SingleSubscriber<T> implements Subscriber<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a SimpleSubscriber you can use to consume the content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Schweet!

@kiiadi kiiadi force-pushed the kiiadi/string-async-request-provider branch from 4cec274 to 4ff4020 Compare September 26, 2017 21:24
kiiadi
kiiadi commented Sep 26, 2017
View reviewed changes
Copy link
Contributor Author

@kiiadi kiiadi left a comment

Choose a reason for hiding this comment

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

Updates made

core/src/main/java/software/amazon/awssdk/async/SingleByteArrayAsyncRequestProvider.java Outdated
Builder bytes(byte[] bytes);
}

public static class DefaultBuilder implements Builder {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following the pattern we used for https://github.com/aws/aws-sdk-java-v2/blob/master/core/src/main/java/software/amazon/awssdk/async/FileAsyncRequestProvider.java

but I'm happy to skip the builder - we can always add it later if we ever want to make this public.

core/src/main/java/software/amazon/awssdk/async/SingleByteArrayAsyncRequestProvider.java Outdated
private final byte[] bytes;

private SingleByteArrayAsyncRequestProvider(DefaultBuilder builder) {
this.bytes = builder.bytes.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's just a reference copy, but since byte is primitive it shouldn't matter.

core/src/test/java/software/amazon/awssdk/async/AsyncRequestProviderTest.java Outdated
}


private abstract class SingleSubscriber<T> implements Subscriber<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Schweet!

@kiiadi
Adding a helper that exposes an async string request provider similar…
dda996e 
… to the AsyncRequestProvider.fromFile implementation.
@kiiadi kiiadi force-pushed the kiiadi/string-async-request-provider branch from 4ff4020 to dda996e Compare September 26, 2017 21:27
varunnvs92
varunnvs92 reviewed Sep 26, 2017
View reviewed changes
core/src/main/java/software/amazon/awssdk/async/AsyncRequestProvider.java
* @see SingleByteArrayAsyncRequestProvider
*/
static AsyncRequestProvider fromString(String string, Charset cs) {
return new SingleByteArrayAsyncRequestProvider(string.getBytes(cs));
Copy link
Contributor

Choose a reason for hiding this comment

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

FileAsyncRequestProvider in this class uses builder pattern. What about doing same for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See @shorea comments. ;) I

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, his comments were hidden due to a new revision of PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good 😄

@kiiadi kiiadi merged commit 7ef04ab into master Sep 27, 2017
@zoewangg zoewangg deleted the kiiadi/string-async-request-provider branch December 6, 2017 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleibman rleibman rleibman approved these changes

@shorea shorea shorea approved these changes

@varunnvs92 varunnvs92 varunnvs92 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kiiadi @rleibman @shorea @varunnvs92