{"action":"created","issue":{"url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/632","repository_url":"https://api.github.com/repos/aws/aws-sdk-java-v2","labels_url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/632/labels{/name}","comments_url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/632/comments","events_url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/632/events","html_url":"https://github.com/aws/aws-sdk-java-v2/pull/632","id":343233081,"node_id":"MDExOlB1bGxSZXF1ZXN0MjAyOTQ0OTc4","number":632,"title":"[PR for discussion]: Use aysnc handlers for sync client","user":{"login":"dagnir","id":261310,"node_id":"MDQ6VXNlcjI2MTMxMA==","avatar_url":"https://avatars2.githubusercontent.com/u/261310?v=4","gravatar_id":"","url":"https://api.github.com/users/dagnir","html_url":"https://github.com/dagnir","followers_url":"https://api.github.com/users/dagnir/followers","following_url":"https://api.github.com/users/dagnir/following{/other_user}","gists_url":"https://api.github.com/users/dagnir/gists{/gist_id}","starred_url":"https://api.github.com/users/dagnir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dagnir/subscriptions","organizations_url":"https://api.github.com/users/dagnir/orgs","repos_url":"https://api.github.com/users/dagnir/repos","events_url":"https://api.github.com/users/dagnir/events{/privacy}","received_events_url":"https://api.github.com/users/dagnir/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-07-20T20:10:11Z","updated_at":"2018-07-20T22:17:37Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/aws/aws-sdk-java-v2/pulls/632","html_url":"https://github.com/aws/aws-sdk-java-v2/pull/632","diff_url":"https://github.com/aws/aws-sdk-java-v2/pull/632.diff","patch_url":"https://github.com/aws/aws-sdk-java-v2/pull/632.patch"},"body":"I'm opening this PR so we can discuss some of the finer points of this change.\r\n\r\nPoints of note and discussion:\r\n\r\n - This changes the retry behavior of streaming responses.\r\n   \r\n   - In the current sync world, customers can throw an instance of `RetryableException` from within [`ResponseTransformer#apply`](https://github.com/aws/aws-sdk-java-v2/blob/ffb8095cab661a1f5318cb217eddde250626e44f/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java#L48-L56) to trigger a retry of the request. This does not work with this change. Rather, it relies completely on the `RetryPolicy` for any retries.\r\n\r\n   - Previously, we would not automatically retry on retryable exceptions (per the `RetryPolicy`). (e.g. when the customer is reading from the response stream). They would need to throw a `RetryableException`. With this change, an `IOException` (or any exception that is retryable according to the `RetryPolicy`) encountered while reading the response stream would trigger a retry.\r\n\r\n   - Using the stream given to `ResponseTransformer#apply` *outside* of the apply method is not well supported. This goes back to the change in retry behavior. If the response tream outlives the `apply` call and a retryable exception is encountered while the stream is being read from, it will trigger a retry operation.\r\n\r\n     From the documentation of the [`ResponseTransformer#needsConnectionLeftOpen`](https://github.com/aws/aws-sdk-java-v2/blob/ffb8095cab661a1f5318cb217eddde250626e44f/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java#L94-L102) it seems like this is a use case we want to support.\r\n\r\n     Personally though, I feel like we could do without this feature.\r\n\r\n\r\n - `InputStreamToPublisher` requires a thread (or at the very least a shared thread pool) in order to emulate a non-blocking `Publisher`. This means that at least for operations that take an `InputStream`, that it would require an extra thread.\r\n\r\nThese are the major changes in behavior that I can see, but I may have missed some.\r\n\r\n\r\nIn this PR I've included the generated `DefaultS3Client` since it has an example of both streaming inputs and streaming response:\r\n\r\n  - [`putObject`](https://github.com/dagnir/aws-sdk-java-v2/blob/b620c89d6de481ec4ccd21c03b3f8bd4c9ef0064/services/s3/src/main/java/software/amazon/awssdk/services/s3/DefaultS3Client.java#L3181-L3200)\r\n\r\n  - [`getObject`](https://github.com/dagnir/aws-sdk-java-v2/blob/b620c89d6de481ec4ccd21c03b3f8bd4c9ef0064/services/s3/src/main/java/software/amazon/awssdk/services/s3/DefaultS3Client.java#L1715-L1737)\r\n"},"comment":{"url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/comments/406740618","html_url":"https://github.com/aws/aws-sdk-java-v2/pull/632#issuecomment-406740618","issue_url":"https://api.github.com/repos/aws/aws-sdk-java-v2/issues/632","id":406740618,"node_id":"MDEyOklzc3VlQ29tbWVudDQwNjc0MDYxOA==","user":{"login":"millems","id":24903526,"node_id":"MDQ6VXNlcjI0OTAzNTI2","avatar_url":"https://avatars0.githubusercontent.com/u/24903526?v=4","gravatar_id":"","url":"https://api.github.com/users/millems","html_url":"https://github.com/millems","followers_url":"https://api.github.com/users/millems/followers","following_url":"https://api.github.com/users/millems/following{/other_user}","gists_url":"https://api.github.com/users/millems/gists{/gist_id}","starred_url":"https://api.github.com/users/millems/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/millems/subscriptions","organizations_url":"https://api.github.com/users/millems/orgs","repos_url":"https://api.github.com/users/millems/repos","events_url":"https://api.github.com/users/millems/events{/privacy}","received_events_url":"https://api.github.com/users/millems/received_events","type":"User","site_admin":false},"created_at":"2018-07-20T22:17:37Z","updated_at":"2018-07-20T22:17:37Z","author_association":"CONTRIBUTOR","body":"I think deviating from the current sync behavior is okay, if it makes it consistent with async. Similarly, we should be willing to amend the async behavior if we feel that it's incorrect.\r\n\r\nCan we support input stream for responses using backpressure and a buffer? If they're reading faster than we're providing, they'll block until they can get the data. If they're reading slower than we're producing, we'll apply backpressure on the stream until they call read? \r\n\r\nSimilarly for requests, can we use their thread to publish the data, since it's just sitting around waiting for the response. Might as well put it to use."}}
{"actor":{"display_login":"millems"}}