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 support for S3TransferManager TransferListeners #2770

Merged
merged 19 commits into from
Oct 21, 2021

Conversation

Bennett-Lynch
Copy link
Contributor

@Bennett-Lynch Bennett-Lynch commented Oct 13, 2021

Revision 4 (4548a53)

Noteworthy changes:

  • Rebase against master
  • Move TransferProgress progress() method from Transfer interface to Upload/Download, as it's not compatible with UploadDirectoryTransfer (will review the interface hierarchy in a follow-up)
  • Since upload(..) and download(..) return an object with a non-nullable TransferProgress, it was difficult to reconcile it with the new try/catch logic that propagated every exception in the form of a CompletableFuture. Adjusted the methods to throw to caller if we cannot instantiate a TransferProgress, and everything else via CompletableFuture.
  • Add CompletableFutureUtils.forwardTransformedResultTo(..) method.

Revision 3 (5a01982)

Noteworthy changes:

  • ProgressPrintingTransferListener converted to public-facing LoggingTransferListener
  • Make LoggingTransferListener's maxTicks configurable
  • Replace source code example in TransferListener documentation with a link to LoggingTransferListener
  • Create StringUtils.repeat(..) method (and add tests)
  • Delegating reaction stream callbacks prefixed with before... or after...
  • Reset bytesTransferred upon re-subscribing (should not be called by current CRT implementation)
  • Move progress-related class to software.amazon.awssdk.transfer.s3.progress package
  • Internal TransferProgressHelper renamed to TransferProgressUpdater
  • Move List<TransferListener> into TransferRequestOverrideConfiguration container class
  • Remove var-args builder methods
  • Simplify TransferProgressUpdater constructors and stop using Optional
  • Rename transferSize to transferSizeInBytes

Deferred for follow-up PRs or conversations:

  • Whether to accept TransferListeners at the client-level or not. Since many TransferListeners are likely to be stateful, it may be counter-intuitive or error-prone to declare them at the client-level. We could accept a Supplier<TransferListener> (i.e., factory), but there's not a strong precedent for that type of API.
  • Which threads to invoke TransferListeners from. Currently, transferInitiated is invoked on the caller's thread, bytesTransferred is invoked by CRT's delivery threads (separate from the CRT event loop), and transferComplete is invoked by the S3NativeClientConfiguration's futureCompletionExecutor. We should make this more consistent, but may require absorbing the recent work that added an Executor for uploadDirectory.

Revision 2 (7d2fe3a)

Noteworthy changes:

  • Add builder support for addListener(..)
  • Move Context class to be an inner-class of TransferListener
  • Correctly annotate TransferListener
  • Expand TransferListener documentation to include the callback lifecycle
  • Convert TransferProgressSnapshot to interface
  • Rename totalBytesTransferred() -> bytesTransferred()
  • Rename totalTransferSize() -> transferSize()
  • Rename totalBytesRemaining() -> bytesRemaining()
  • Remove percentageTransferred() in favor of only exposing ratioTransferred()
  • Make ratioTransferred() return 1.0 (100%) for zero-byte objects
  • Prefix internal AsyncRequestBodyListener/AsyncResponseTransformerListener methods with delegated
  • Change TransferListenerInvoker to log on behalf of TransferListener

Unresolved:

  • Keep ProgressPrintingTransferListener private or make public
  • Keep Mutable annotation or remove
  • Make listeners part of TransferRequestOverrideConfiguration or just TransferRequest
  • Accept TransferListeners at the TM/client-level or only request-level
  • Determine what type of thread guarantees we wish to offer listener implementations

Revision 1 (9976e29)

This adds initial support for S3TransferManager progress listeners. The motivation and design is consistent as outlined in #2729. It also addresses some customer asks as mentioned in #37.

Every @SdkPublicApi has been thoroughly documented with its description and usage instructions where applicable.

Furthermore, there is a temporary tmp.md file that captures some ongoing development thoughts and notes. This file will be fully addressed & deleted before merging to master.

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

Copy link
Contributor

@zoewangg zoewangg left a comment

Choose a reason for hiding this comment

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

Still going through the PR.

1. Are we correct to distinguish between `ratioTransferred` vs `percentageTransferred`? Will the return value of `percentageTransferred` surprise any users?
1. Should the `Context` wrapper class reside inside `TransferListener` or outside? Outside is consistent w/ `ExecutionInterceptor`'s `Context`, but prevents having 2 `Context` objects in the same package.
1. Should we have a callback for when the `content-length` is discovered on a Get?
1. Also provide option to declare listeners at the TM/client level?
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this, had a comment earlier on before seeing this

@Bennett-Lynch Bennett-Lynch changed the title Add support for S3TransferManager progress listeners Add support for S3TransferManager TransferListeners Oct 19, 2021
@Bennett-Lynch Bennett-Lynch marked this pull request as ready for review October 21, 2021 07:09
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 20 Code Smells

82.1% 82.1% Coverage
3.0% 3.0% Duplication

@Bennett-Lynch Bennett-Lynch merged commit da53f77 into aws:master Oct 21, 2021
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