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

Add more options for FileAsyncResponseTransformer #1222

Closed
wants to merge 1 commit into from

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Apr 23, 2019

Description

Add the following things to control the behavior of the transformer:

  • position parameter to specify where in the file to write the stream
  • isNewFile parameter to specify whether this is a new file
  • deleteOnFailure to specify whether the file should be deleted on failure

Motivation and Context

File opening flexibility.

Testing

Added new tests.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@codecov-io
Copy link

codecov-io commented Apr 23, 2019

Codecov Report

Merging #1222 into master will increase coverage by 0.24%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1222      +/-   ##
============================================
+ Coverage     59.02%   59.27%   +0.24%     
- Complexity     4632     4651      +19     
============================================
  Files           747      747              
  Lines         23183    23197      +14     
  Branches       1737     1738       +1     
============================================
+ Hits          13684    13749      +65     
+ Misses         8805     8743      -62     
- Partials        694      705      +11
Impacted Files Coverage Δ Complexity Δ
...in/java/software/amazon/awssdk/utils/Validate.java 63.15% <0%> (-1.71%) 49 <0> (ø)
...on/awssdk/core/async/AsyncResponseTransformer.java 50% <100%> (+50%) 2 <1> (+2) ⬆️
...e/internal/async/FileAsyncResponseTransformer.java 73.07% <87.5%> (+73.07%) 13 <5> (+13) ⬆️
...on/awssdk/services/kinesis/KinesisRetryPolicy.java 85.71% <0%> (ø) 2% <0%> (-1%) ⬇️
...are/amazon/awssdk/core/async/AsyncRequestBody.java 36.36% <0%> (+9.09%) 4% <0%> (+1%) ⬆️
.../netty/internal/UnusedChannelExceptionHandler.java 47.05% <0%> (+29.41%) 6% <0%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af22896...9749ff7. Read the comment docs.

@dagnir
Copy link
Contributor Author

dagnir commented Apr 23, 2019

Missed the default delete-on-error behavior. I'll need to fix that.

@dagnir dagnir force-pushed the file-async-transformer-options branch from 479b363 to 899da25 Compare April 23, 2019 19:40
@dagnir dagnir changed the title Position and new file flag for file transformer Add more options for FileAsyncResponseTransformer Apr 23, 2019
Add the following things to control the behavior of the transformer:

 - position parameter to specify where in the file to write the stream
 - isNewFile parameter to specify whether this is a new file
 - deleteOnFailure to specify whether the file should be deleted on failure
@dagnir dagnir force-pushed the file-async-transformer-options branch from 899da25 to 9749ff7 Compare April 23, 2019 19:51
* @return AsyncResponseTransformer instance.
*/
static <ResponseT> AsyncResponseTransformer<ResponseT, ResponseT> toFile(Path path,
long position,
Copy link
Contributor

@zoewangg zoewangg Apr 23, 2019

Choose a reason for hiding this comment

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

I wonder if we should use a request wrapper for this. Something like toFile(Path path, FileOption option)? My concern is that we might need to add other parameters in the future.

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 think in that case case a Builder might be a better option, something like

AsyncResponseTransformer.toFileTransformerBuilder()
                        .position(...)
                        .deleteOnFailure(...)
                        .build();

We have one for FileAsyncRequestBody

public interface Builder extends SdkBuilder<Builder, FileAsyncRequestBody> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Like the idea of builder but I'm not sure if we want to put toFileTransformerBuilder api to AsyncResponseTransformer.

What would toFile look like? toFile(FileAsyncResponseTransformer.Builder 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.

We can probably just keep toFile(Path) as-is, since it's behavior is probably what most customers want. This extra stuff is for less common/more specialized cases I would think (such as use in the transfer manager).

@dagnir
Copy link
Contributor Author

dagnir commented Mar 31, 2020

Closing this for now.

@dagnir dagnir closed this Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants